Closed Bug 1423659 Opened 2 years ago Closed 2 years ago

HTML5 video is requesting the same range multiple times.

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: haroldoop, Assigned: bechen)

References

Details

(Keywords: compat)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

Tried to play a video through a custom HTTP proxy.



Actual results:

Basically, it's the bug described in: https://stackoverflow.com/questions/47471982/html5-video-loads-slowly-on-firefox-but-its-pretty-fast-on-chrome

If played from the start, the video takes a long time to load. Also, for some reason, FireFox is asking the same range multiple times when playing from the start. 

If the video is skipped forward a few seconds, it plays normaly.

The same setup has been tested on Chrome and Edge, and the video plays normally on those.


Expected results:

The video should have played correctly.
Benjamin, 
It is normal to ask the same range multiple times?
Flags: needinfo?(bechen)
Keywords: compat
(In reply to Blake Wu [:bwu][:blakewu](OoO until 12/6) from comment #1)
> Benjamin, 
> It is normal to ask the same range multiple times?
Of course abnormal. 

If I understand correctly, we only fire "request range" when we open a channel, => at the beginning we open a channel or seek operation.
So if the issue is about the same range, that means probably we seek to the same position multiple times.

It's better if there is a website/link that we can reproduce the issue.
Flags: needinfo?(bechen)
Some additional info: I have also experimented with serving the video statically via Apache; it works if accessing locally, but if I use ngrok (https://ngrok.com/) to be access it externally, exactly the same bug happens.

I have created a temporary link to this setup at http://9e1f9fc2.ngrok.io/teste-temp/teste_1_digitacao.mp4, but I'm not sure how long it wil stay up.
(In reply to Haroldo de Oliveira Pinheiro from comment #3)
> Some additional info: I have also experimented with serving the video
> statically via Apache; it works if accessing locally, but if I use ngrok
> (https://ngrok.com/) to be access it externally, exactly the same bug
> happens.
> 
> I have created a temporary link to this setup at
> http://9e1f9fc2.ngrok.io/teste-temp/teste_1_digitacao.mp4, but I'm not sure
> how long it wil stay up.

Thanks for the help.
I download the file and it can play smoothly as local file. So the problem may not about the decoding.

The video is lag at the beginning and I can see the ChannelMediaResource::SetupChannelHeaders() be called many times, also our MDSM go into BufferingState.
Assignee: nobody → bechen
Problem seems here
https://searchfox.org/mozilla-central/source/dom/media/mp4/MoofParser.cpp#178

//
for (Box box(&context, mOffset); box.IsAvailable(); box = box.Next())
//

The MediaCacheStream::ReadAt be called twice at one line code....

Stream 0x7f6010a807a8 ReadAt 0 
Stream 0x7f6010a807a8 Read at 0 count=8192
Stream 0x7f6010a807a8 ReadAt 4332917
example: play a mp4 file whose moov box be placed at the end of file and the ftyp box at the beginning of the file. 

https://searchfox.org/mozilla-central/source/dom/media/mp4/MoofParser.cpp#205
The ScanForMetadata() seek to the end to find moov box.

https://searchfox.org/mozilla-central/source/dom/media/mp4/MoofParser.cpp#226
Seek to beginning to read the ftyp box.

https://searchfox.org/mozilla-central/source/dom/media/mp4/MoofParser.cpp#231
Seek to end to read the moov box.

Finally seek to beginning to decode the first frame.

I'm not sure the scenario is exactly the same as the reporter's issue, because the moov of reporter's file is not placed at the end of file, but I'll try to write a patch reduce the unnecessary seek.
Comment on attachment 8940048 [details]
Bug 1423659 - Remove ftyp and HasMetadata().

https://reviewboard.mozilla.org/r/210314/#review216048


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/mp4/MoofParser.cpp:221
(Diff revision 1)
>  bool
>  MoofParser::HasMetadata()
>  {
> -  MediaByteRange ftyp;
> -  MediaByteRange moov;
> -  ScanForMetadata(ftyp, moov);
> +  if (mFtypBox.IsAvailable() && mMoovBox.IsAvailable()) {
> +    return true;
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8940048 [details]
Bug 1423659 - Remove ftyp and HasMetadata().

https://reviewboard.mozilla.org/r/210314/#review216988

We don't need 'ftyp' and MoofParser::HasMetadata() anymore, the whole algorithm should be easier without these things.

::: dom/media/mp4/MoofParser.cpp:178
(Diff revision 2)
>    RefPtr<BlockingStream> stream = new BlockingStream(mSource);
>  
>    BoxContext context(stream, byteRanges);
>    for (Box box(&context, mOffset); box.IsAvailable(); box = box.Next()) {
>      if (box.IsType("ftyp")) {
> -      aFtyp = box.Range();
> +      RefPtr<MediaByteBuffer> data = new MediaByteBuffer();

I think we don't need 'ftyp' box, it is for stagefright. Rust MP4 parser could parse 'moov' only.

::: dom/media/mp4/MoofParser.cpp:224
(Diff revision 2)
> -  MediaByteRange ftyp;
> -  MediaByteRange moov;
> -  ScanForMetadata(ftyp, moov);
> -  return !!ftyp.Length() && !!moov.Length();
> +  if (mFtypBox.IsAvailable() && mMoovBox.IsAvailable()) {
> +    return true;
> +  }
> +  ReadMetadata();
> +  return (mFtypBox.IsAvailable() && mMoovBox.IsAvailable());
>  }

We don't need this function anymore.
Attachment #8940048 - Flags: review?(ayang)
Attachment #8941000 - Attachment is obsolete: true
Attachment #8941000 - Flags: review?(ayang)
Comment on attachment 8940048 [details]
Bug 1423659 - Remove ftyp and HasMetadata().

https://reviewboard.mozilla.org/r/210314/#review217372
Attachment #8940048 - Flags: review?(ayang) → review+
See Also: → 1336271
Keywords: checkin-needed
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0932f3bbbc1a
Remove ftyp and HasMetadata(). r=alfredo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0932f3bbbc1a
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.