Closed Bug 1128380 Opened 6 years ago Closed 6 years ago

Make MediaDecoderStateMachine's video queue length dynamic

Categories

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

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Priority: -- → P2
Add IsHardwareAccelerated() implementation for AppleVDA/VT and AVCC wrapper.
Attachment #8568451 - Flags: review?(cpearce)
Attachment #8568451 - Flags: review?(cpearce) → review+
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.
Attachment #8568918 - Flags: review?(cpearce)
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.
Attachment #8568918 - Flags: review?(cpearce) → review+
(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
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.
Attachment #8568956 - Flags: review?(cpearce)
Attachment #8568918 - Attachment is obsolete: true
Attachment #8568956 - Flags: review?(cpearce) → review+
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.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/8cdf05d1d04d
https://hg.mozilla.org/mozilla-central/rev/9506fff39d1c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.