1128380 - Make MediaDecoderStateMachine's video queue length dynamic

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We need to change MediaDecoderStateMachine's mAmpleVideoFrames field to being a function that can return a dynamic value. This is because on Mac with MSE we may end up switching to software decoding for some low res segments and then switching back to hardware decoding for some other high res segments. So just assuming the value is static once reading metadata is complete like we do now won't cut it.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Add IsHardwareAccelerated implementation for AVCC and mac decoder

Add IsHardwareAccelerated() implementation for AppleVDA/VT and AVCC wrapper.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Make AmpleVideoFrames calculation dynamic

Make the calculation of 'AmpleVideoFrames' dynamic ; export the detection of hw acceleration to the MediaDecoderReader. Move the calculation of max(sVideoQueueHWAccelSize/sVideoQueueDefaultSize) in the constructor so it's not done over and over again.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8568918 [details] [diff] [review]
Make AmpleVideoFrames calculation dynamic

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

r+ with nits.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +258,5 @@
>                                   MAX_VIDEO_QUEUE_SIZE);
>      Preferences::AddUintVarCache(&sVideoQueueHWAccelSize,
>                                   "media.video-queue.hw-accel-size",
>                                   MIN_VIDEO_QUEUE_SIZE);
> +    sVideoQueueDefaultSize =

We're adding a pref cache to sVideoQueueDefaultSize and sVideoQueueHWAccelSize a few lines above. This means when the pref changes, we'll write the new value into sVideoQueueDefaultSize and sVideoQueueHWAccelSize. So the values you're assigning to here could be overwritten if the pref changes.

So you'll need to do the min/max check in GetAmpleVideoFrames().

::: dom/media/mediasource/MediaSourceReader.h
@@ +146,5 @@
>             (!GetVideoReader() || GetVideoReader()->IsAsync());
>    }
>  
> +  virtual bool VideoIsHardwareAccelerated() const MOZ_OVERRIDE {
> +    return GetVideoReader() && GetVideoReader()->VideoIsHardwareAccelerated();

This can be called on threads other than the decode thread. That means this function needs to be threadsafe.

You're only threadsafe here because the caller happens to hold the monitor while calling this, else the video reader could switch while we're this. You should either take the monitor, or assert that the monitor is held in this function.

IsAsync() above should also have an assertion, can you add that please, I recall noting this previously but forgot to follow up on it.

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8568918 [details] [diff] [review]
> Make AmpleVideoFrames calculation dynamic
> 
> Review of attachment 8568918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits.
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +258,5 @@
> >                                   MAX_VIDEO_QUEUE_SIZE);
> >      Preferences::AddUintVarCache(&sVideoQueueHWAccelSize,
> >                                   "media.video-queue.hw-accel-size",
> >                                   MIN_VIDEO_QUEUE_SIZE);
> > +    sVideoQueueDefaultSize =
> 
> We're adding a pref cache to sVideoQueueDefaultSize and
> sVideoQueueHWAccelSize a few lines above. This means when the pref changes,
> we'll write the new value into sVideoQueueDefaultSize and
> sVideoQueueHWAccelSize. So the values you're assigning to here could be
> overwritten if the pref changes.
> 
> So you'll need to do the min/max check in GetAmpleVideoFrames().
> 

Ah ok... I didn't know AddUintVarCache did that, I thought it was only going to be initialised in the constructor once.

> You're only threadsafe here because the caller happens to hold the monitor
> while calling this, else the video reader could switch while we're this. You
> should either take the monitor, or assert that the monitor is held in this
> function.
> 
> IsAsync() above should also have an assertion, can you add that please, I
> recall noting this previously but forgot to follow up on it.

will do

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Make AmpleVideoFrames calculation dynamic

Carrying r+; the only place that needs locking are the Media Source implementations ; I think leaving the actual implementation to handle being thread-safe is best. No point asserting if all they do is return a static boolean.

Comment 6

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/72a843ecfb92
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d97bad42411

Comment 7

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7235774&repo=mozilla-inbound

Comment 8

[Jean-Yves Avenard [:jya]]

It's the same problem that cause bug 1139380 intermittently. So it's not a new bug :(

We just happen to crash earlier. For some reason mDecoder is nullptr.

Comment 9

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdf05d1d04d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9506fff39d1c

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cdf05d1d04d
https://hg.mozilla.org/mozilla-central/rev/9506fff39d1c