Make MediaDecoderStateMachine's video queue length dynamic

RESOLVED FIXED in Firefox 39

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: jya)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
x86_64
macOS
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.

Updated

4 years ago
Priority: -- → P2
(Assignee)

Comment 1

4 years ago
Add IsHardwareAccelerated() implementation for AppleVDA/VT and AVCC wrapper.
Attachment #8568451 - Flags: review?(cpearce)
(Reporter)

Updated

4 years ago
Attachment #8568451 - Flags: review?(cpearce) → review+
(Assignee)

Comment 2

4 years ago
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)
(Reporter)

Comment 3

4 years ago
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+
(Assignee)

Comment 4

4 years ago
(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
(Assignee)

Comment 5

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #8568918 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8568956 - Flags: review?(cpearce) → review+
(Assignee)

Comment 8

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

4 years ago
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.