Closed Bug 1418430 Opened 2 years ago Closed 2 years ago

Range header in HTML5 video

Categories

(Core :: Audio/Video: Playback, defect, P2)

57 Branch
defect

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
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?
>> 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.
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/
Attachment #8930788 - Flags: review?(bechen)
Attachment #8930789 - Flags: review?(bechen)
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 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+
Attachment #8930788 - Flags: review?(gsquelart)
Attachment #8930789 - Flags: review?(gsquelart)
Assignee: nobody → jwwang
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 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+
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.
Thanks for the reviews!
Status: UNCONFIRMED → ASSIGNED
Component: Audio/Video → Audio/Video: Playback
Ever confirmed: true
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
https://hg.mozilla.org/mozilla-central/rev/c5e45c611460
https://hg.mozilla.org/mozilla-central/rev/8347ecd0911b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
(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.
(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?
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.
Depends on: 1441153
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"
No longer depends on: 1441153
Depends on: 1441153
You need to log in before you can comment on or make changes to this bug.