Closed
Bug 1418430
Opened 7 years ago
Closed 7 years ago
fetch again after incomplete Partial Content Content-Range response in HTML5 video
Categories
(Core :: Audio/Video: Playback, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: elatllat, Assigned: jwwang)
References
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36
Steps to reproduce:
Return only 1 MB of video per request.
I though I had this working in 2015 but it's not a recent regression.
Actual results:
Firefox asks for the next range but it's the wrong one (131,072-) and then firefox gives up.
eg:
0- 0- 999999/12498473
12419072- 12419072-12498472/12498473
131072- 131072- 1131071/12498473
Expected results:
Firefox should ask for the correct range (1,032,768-) and keep asking for ranges as required, like Chrome.
eg:
0- 0- 999999/12498473
12419072- 12419072-12498472/12498473
1032768- 1032768- 2032767/12498473
2032768- 2032768- 3032767/12498473
3032768- 3032768- 4032767/12498473
etc
Reporter | ||
Comment 1•7 years ago
|
||
Firefox is the odd one out as this works on Edge and Internet Explorer as well as Chrome.
(In reply to elatllat from comment #0)
> User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0)
> AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36
>
> Steps to reproduce:
>
> Return only 1 MB of video per request.
> I though I had this working in 2015 but it's not a recent regression.
The meaning of your columns of numbers is not obvious. Can you provide a link to reproduce this issue?
Reporter | ||
Comment 3•7 years ago
|
||
>> Firefox should ask for the correct range...
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #2)
> The meaning of your columns of numbers is not obvious....
The columns are (respectively) requested and returned ranges ( https://tools.ietf.org/html/rfc7233 ).
I'll work on providing an example link.
Reporter | ||
Comment 4•7 years ago
|
||
Example Link(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #2)
> Can you provide a link to reproduce this issue?
http://23.253.41.15/ff/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8930788 -
Flags: review?(bechen)
Attachment #8930789 -
Flags: review?(bechen)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8930789 [details]
Bug 1418430. P2 - simplify the if statement of "reopen on error".
https://reviewboard.mozilla.org/r/201870/#review207262
Attachment #8930789 -
Flags: review?(bechen) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8930788 [details]
Bug 1418430. P1 - always check "reopen on error" when a connection is closed.
https://reviewboard.mozilla.org/r/201868/#review207266
Attachment #8930788 -
Flags: review?(bechen) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8930788 -
Flags: review?(gsquelart)
Attachment #8930789 -
Flags: review?(gsquelart)
Assignee: nobody → jwwang
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8930788 [details]
Bug 1418430. P1 - always check "reopen on error" when a connection is closed.
https://reviewboard.mozilla.org/r/201868/#review207538
Attachment #8930788 -
Flags: review?(gsquelart) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8930789 [details]
Bug 1418430. P2 - simplify the if statement of "reopen on error".
https://reviewboard.mozilla.org/r/201870/#review207578
::: dom/media/MediaCache.cpp:2155
(Diff revision 1)
> + * +--------+-----------+----------+
> + * | -1 | 0 | yes |
> + * +--------+-----------+----------+
> + * | -1 | > 0 | seekable |
> + * +--------+-----------+----------+
> + * | 0 | X | no |
This table is really useful! Was the ascii art needed? :-)
This case (length==0 -> don't reopen) doesn't match the code for a non-0 offset and a seekable transport:
`(offset == 0 || seekable) && offset != length` => `(false || true) && true` => true.
It could be fixed by adding `&& mStreamLength != 0` (assuming the table is correct).
Attachment #8930789 -
Flags: review?(gsquelart) → review+
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8930789 [details]
Bug 1418430. P2 - simplify the if statement of "reopen on error".
https://reviewboard.mozilla.org/r/201870/#review207578
> This table is really useful! Was the ascii art needed? :-)
>
> This case (length==0 -> don't reopen) doesn't match the code for a non-0 offset and a seekable transport:
> `(offset == 0 || seekable) && offset != length` => `(false || true) && true` => true.
> It could be fixed by adding `&& mStreamLength != 0` (assuming the table is correct).
No. plus sign '+', vertical bar '|' and dash '-' are all you need to make a decent table.
https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/media/MediaCache.cpp#2068-2070
mStreamLength is adjusted as mChannelOffset advances. So we always have mChannelOffset <= mStreamLength if mStreamLength >= 0. When mStreamLength is 0, the only valid value for mChannelOffset is also 0.
Assignee | ||
Comment 12•7 years ago
|
||
Thanks for the reviews!
Status: UNCONFIRMED → ASSIGNED
Component: Audio/Video → Audio/Video: Playback
Ever confirmed: true
Comment 13•7 years ago
|
||
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5e45c611460
P1 - always check "reopen on error" when a connection is closed. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/8347ecd0911b
P2 - simplify the if statement of "reopen on error". r=bechen,gerald
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c5e45c611460
https://hg.mozilla.org/mozilla-central/rev/8347ecd0911b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Reporter | ||
Comment 15•7 years ago
|
||
Thanks, build "59.0a1 (2017-11-23)" can play the whole video now.
There is a stuttering issue with the implementation because FireFox is waiting until it's all out of data before fetching the next range. It would be better to fetch data when the remaining data is less than the total and less than the last range size (effectively a cache/buffer).
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to elatllat from comment #15)
> Thanks, build "59.0a1 (2017-11-23)" can play the whole video now.
>
> There is a stuttering issue with the implementation because FireFox is
> waiting until it's all out of data before fetching the next range. It would
> be better to fetch data when the remaining data is less than the total and
> less than the last range size (effectively a cache/buffer).
Let's wait to see if bug 1419668 fix the stuttering for you.
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to elatllat from comment #15)
> Thanks, build "59.0a1 (2017-11-23)" can play the whole video now.
>
> There is a stuttering issue with the implementation because FireFox is
> waiting until it's all out of data before fetching the next range. It would
> be better to fetch data when the remaining data is less than the total and
> less than the last range size (effectively a cache/buffer).
Can you go to about:config and change "media.memory_cache_max_size" to 1 to see if it works for you?
Reporter | ||
Comment 18•7 years ago
|
||
I'm not sure if the nightly updated again since I last checked but now there is a crash every second page view, and the cached content indicator in the video progress bar is jumping all over the place. The video did not stutter when it did work though. I did not edit the config. I did send 2 crash reports.
Reporter | ||
Comment 19•6 years ago
|
||
The example seems to work in Firefox 60.0.2.
But the range is overly greedy and requests the whole file from the server instead of as "on demand" like Chromium.
"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0"
Updated•3 years ago
|
Summary: Range header in HTML5 video → fetch again after incomplete Partial Content Content-Range response in HTML5 video
You need to log in
before you can comment on or make changes to this bug.
Description
•