1199518 - WebMDemuxer not properly calculating next keyframe time when data isn't buffered

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

The logic in WebMDemuxer::GetNextKeyframeTime() is wrong.

It relies on the WebMBufferedState to know where the next keyframe time is. But the WebMBufferedState is only valid for buffered data and notified via NotifyDataArrived().

While this assumption will always work with MSE , it is not guaranteed with plain webm unless the file is local.

The only guaranteed way to find the next keyframe is to keep demuxing until we found one.

The bug will lead to stall when playing data that isn't buffered and the state machine has found that playback is too slow and we should skip to the next keyframe.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P1. Have WebMDemuxer use its own logger.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P2. Properly determine next keyframe time.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8659090 [details] [diff] [review]
[webm] P2. Properly determine next keyframe time.

Review of attachment 8659090 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webm/WebMDemuxer.cpp
@@ +34,5 @@
>  PRLogModuleInfo* gWebMDemuxerLog = nullptr;
>  extern PRLogModuleInfo* gNesteggLog;
>  
> +// How far ahead will we look when searching future keyframe. In microseconds
> +#define MAX_LOOK_AHEAD 10000000

Is there a specific reason for this to be 10s or is it an arbitrary number?  Please state either way in the comment.

@@ +914,5 @@
> +    sample = mSamples.PopFront();
> +    skipSamplesQueue.Push(sample);
> +  }
> +
> +  while(skipSamplesQueue.GetSize()) {

Space between while and (

@@ +917,5 @@
> +
> +  while(skipSamplesQueue.GetSize()) {
> +    sample = skipSamplesQueue.PopFront();
> +    mSamples.Push(sample);
> +  }

It's a shame we can't simply replace mSamples with skipSamplesQueue.

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Matthew Gregan [:kinetik] from comment #3)
> Comment on attachment 8659090 [details] [diff] [review]
> [webm] P2. Properly determine next keyframe time.
> 
> Review of attachment 8659090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webm/WebMDemuxer.cpp
> @@ +34,5 @@
> >  PRLogModuleInfo* gWebMDemuxerLog = nullptr;
> >  extern PRLogModuleInfo* gNesteggLog;
> >  
> > +// How far ahead will we look when searching future keyframe. In microseconds
> > +#define MAX_LOOK_AHEAD 10000000
> 
> Is there a specific reason for this to be 10s or is it an arbitrary number? 
> Please state either way in the comment.

it's a number coming out of cpearce's bottom :)

youtube's video key frame appears to be 0.8-1.5s appart.
All webm videos I played with either have them at around 2s or none at all (a single keyframe at the beginning)

Do you have another suggestion?

> It's a shame we can't simply replace mSamples with skipSamplesQueue.

maybe I could implement the operator= instead.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> it's a number coming out of cpearce's bottom :)
> 
> youtube's video key frame appears to be 0.8-1.5s appart.
> All webm videos I played with either have them at around 2s or none at all
> (a single keyframe at the beginning)
> 
> Do you have another suggestion?

No suggestion, just add something small to the comment to document that it's a bit of a guess based on observed behaviour (but feel free to leave cpearce's posterior out of it!).

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P2. Properly determine next keyframe time.

Add comment on how the MAX_LOOK_AHEAD came to be.
Implement various operators in MediaRawDataQueue so we don't need to loop doing it all at once.

Amend the logs to show how long we've buffered while searching for the keyframe.

Carrying r+

Comment 7

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02841936fcd7

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1794ed046155
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72d4bfd2811

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8659712 [details] [diff] [review]
[webm] P2. Properly determine next keyframe time.

Approval Request Comment
[Feature/regressing bug #]:1148102
[User impact if declined]: Invalid playback ; stalls. Would have to disable the new WebM player and revert to the old one which would reintroduce several bugs
[Describe test coverage new/current, TreeHerder]: Local tests, try runs.
[Risks and why]: Low ; this is properly implementing the API
[String/UUID change made/needed]: None

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1794ed046155
https://hg.mozilla.org/mozilla-central/rev/f72d4bfd2811

Comment 11

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f258d3b40169
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/46f81b68c55f