Closed Bug 794747 Opened 8 years ago Closed 8 years ago

Allow adjusting the size of the MediaQueue to consume less memory

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(2 files, 1 obsolete file)

The MediaQueue in nsBuiltinDecoderReader.cpp/h has a fixed size of 10 images.

In some cases we don't need that many images. It would be useful to have a way to adjust the size of the MediaQueue. Adding a pref would be a good start.
As mentioned in bug 773440, the pref has to be accessed from the main thread.

On top of this, we could let the nsBuiltinDecoderReader adjust dynamically the size in function of the latency. Here is the relevant quote from roc in bug 773440:
> We could probably autotune the value based on the observed latencies of deocding a 
> particular video stream, e.g. start conservatively and once we've decoded a few seconds
> of video, make the queue length 50% bigger than the largest latency we've seen so far.
(In reply to Nicolas Silva [:nical] from comment #0)
> The MediaQueue in nsBuiltinDecoderReader.cpp/h has a fixed size of 10 images.

That's AMPLE_VIDEO_FRAMES.

> As mentioned in bug 773440, the pref has to be accessed from the main thread.

Note that there's no need to remote it, you can safely read the pref in nsBuiltinDecoderStateMachine's constructor, which already reads media.realtime_decoder.enabled.
The name of the pref is media.video-queue.default-size, I set it to 10 by default to not change the current behaviour.
Attachment #665294 - Flags: review?(kinetik)
arf, made typo. it's late.
Attachment #665294 - Attachment is obsolete: true
Attachment #665294 - Flags: review?(kinetik)
Attachment #665295 - Flags: review?(kinetik)
Comment on attachment 665295 [details] [diff] [review]
Add a pref to set the default size of the videoqueue

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

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +416,5 @@
>      mRealTime = false;
>  
>    mBufferingWait = mRealTime ? 0 : BUFFERING_WAIT_S;
>    mLowDataThresholdUsecs = mRealTime ? 0 : LOW_DATA_THRESHOLD_USECS;
> +  mAmpleVideoFrames = Preferences::GetInt("media.video-queue.default-size", 10);

GetUint to match the type of mAmpleVideoFrames.

A video queue size of 0 is probably bad, and videoPumpThreshold is computed as half mAmpleVideoFrames, so let's restrict the minimum queue length to 2.
Attachment #665295 - Flags: review?(kinetik) → review+
You'll need to update this on top of bug 759506, which uses AMPLE_VIDEO_FRAMES = 3 on Gonk.

I missed the typo for "enqueued" in all.js, please fix that too.
Isn't the default pref in all.js going to clobber the Gonk-specific value?
gosh! you are right, If I set the pref in b2g.js, it will override all.js, right?
Attachment #666013 - Flags: review?
Attachment #666013 - Flags: review? → review?(kinetik)
Attachment #666013 - Flags: review?(kinetik) → review+
(In reply to Nicolas Silva [:nical] from comment #8)
> gosh! you are right, If I set the pref in b2g.js, it will override all.js,
> right?

Yep, we shrink the media cache size on B2G the same way.
https://hg.mozilla.org/mozilla-central/rev/05684d6424cb
https://hg.mozilla.org/mozilla-central/rev/cfca07bb3a34
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.