1418430 - fetch again after incomplete Partial Content Content-Range response in HTML5 video

Status: VERIFIED FIXED

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

Description

[[e-mail]]

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

Comment 1

[[e-mail]]

Firefox is the odd one out as this works on Edge and Internet Explorer as well as Chrome.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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?

Comment 3

[[e-mail]]

>> 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.

Comment 4

[[e-mail]]

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 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1418430. P1 - always check "reopen on error" when a connection is closed.

The server might send us fewer bytes than requested. So we also need
"reopen on error" in this case as well.

Review commit: https://reviewboard.mozilla.org/r/201868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201868/

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1418430. P2 - simplify the if statement of "reopen on error".

A truth table is listed to illustrate all conditions of length/offset/reopen.

The original code doesn't handle the following cases correctly:
1. length == offset == 0, shouldn't reopen the channel for there is no data to download.
2. length == -1 && offset > 0, should reopen the channel if seekable.

Review commit: https://reviewboard.mozilla.org/r/201870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201870/

Comment 7

[Benjamin Chen [:bechen]]

Comment on attachment 8930789 [details]
Bug 1418430. P2 - simplify the if statement of "reopen on error".

https://reviewboard.mozilla.org/r/201870/#review207262

Comment 8

[Benjamin Chen [:bechen]]

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

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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).

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the reviews!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c5e45c611460
https://hg.mozilla.org/mozilla-central/rev/8347ecd0911b

Comment 15

[[e-mail]]

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).

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(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?

Comment 18

[[e-mail]]

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.

Comment 19

[[e-mail]]

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"