Closed Bug 1414759 Opened 3 years ago Closed 3 years ago

Streamline MediaPrefs

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

MediaPrefs has some dead prefs and some prefs that could just be code constants.
Attachment #8925449 - Flags: review?(cpearce)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
cpearce, here is a list of all the numeric MediaPrefs that aren't changed by any tests, don't seem like something a user would need to change, and so which are good candidates for conversion into C++ code constants. What do you think?

- "media.memory_caches_combined_limit_pc_sysmem"
- "media.cache.resource-index"
- "media.cache_resume_threshold"
- "media.cache_readahead_limit"
- "media.resampling.rate"
- "media.wmf.decoder.thread-count"
- "media.num-decode-threads"
- "media.decoder.limit"

I haven't considered whether any of the non-dead bool MediaPrefs could be removed, but I'd be happy to hear if you have any suggestions there. ("media.gmp.insecure.allow" and "media.ruin-av-sync.enabled" are two intriguing ones :)
Flags: needinfo?(cpearce)
Component: Audio/Video → Audio/Video: Playback
Comment on attachment 8925449 [details] [diff] [review]
Remove dead media prefs

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

::: dom/ipc/ContentPrefs.cpp
@@ -150,5 @@
>    "media.ffvpx.enabled",
>    "media.ffvpx.low-latency.enabled",
>    "media.flac.enabled",
>    "media.forcestereo.enabled",
>    "media.gmp.async-shutdown-timeout",

media.gmp.async-shutdown-timeout and media.gmp.decoder.enabledcan be removed too; they're unused.
Attachment #8925449 - Flags: review?(cpearce) → review+
(In reply to Nicholas Nethercote [:njn] from comment #2)
> cpearce, here is a list of all the numeric MediaPrefs that aren't changed by
> any tests, don't seem like something a user would need to change, and so
> which are good candidates for conversion into C++ code constants. What do
> you think?
> 

> - "media.memory_caches_combined_limit_pc_sysmem"
This one can be a constant.

> - "media.cache.resource-index"
This one can be a constant.

> - "media.cache_resume_threshold"
This one I feel should be user-toggleable.

> - "media.cache_readahead_limit"
This one I feel should be user-toggleable.

> - "media.resampling.rate"
This one can be a constant.

> - "media.wmf.decoder.thread-count"
This one can be a constant.

> - "media.num-decode-threads"
This one can be a constant.


> - "media.decoder.limit"
This one can be a constant.



> 
> I haven't considered whether any of the non-dead bool MediaPrefs could be
> removed, but I'd be happy to hear if you have any suggestions there.
> ("media.gmp.insecure.allow" and "media.ruin-av-sync.enabled" are two
> intriguing ones :)

These are for testing.

media.ruin-av-sync.enabled causes the decode path to not drop frames that are late. This means we'll paint frames that are late, so A/V sync will be out. When the decode can't keep up video frames often come out of the decode pipeline after they're supposed to keep painting. There's a talos test that counts how many frames we can render per second. When I fixed our A/V sync logic to drop those frames, we got a talos regression, as we don't paint as many frames anymore (but the ones we do paint, we paint in time with the audio). So to placate the people who care about such things, we added a pref so that the gfx team can continue to test how fast their pipeline is at painting (the wrong thing).

media.gmp.insecure.allow allows running of GMPs (i.e. OpenH264) on old versions of Linux that don't support sandboxing. It allows people to opt-in to running GMPs on systems where we can't guarantee the sandbox protects them.
Flags: needinfo?(cpearce)
> media.gmp.async-shutdown-timeout and media.gmp.decoder.enabled can be removed too; they're unused.

media.gmp.decoder.enabled is used via the PDMGMPEnabled value.
Specifically:
- media.decoder.limit
- media.num-decode-threads
- media.resampling.rate
- media.wmf.decoder.thread-count
- media.cache.resource-index
Attachment #8927691 - Flags: review?(cpearce)
Comment on attachment 8927691 [details] [diff] [review]
Replace some unnecessary media prefs with code constants

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

r=cpearce with comment addressed.

::: dom/media/MediaResource.cpp
@@ +41,5 @@
>  
>  NS_IMPL_ADDREF(MediaResource)
>  NS_IMPL_RELEASE_WITH_DESTROY(MediaResource, Destroy())
>  
> +static const uint32_t kMediaResourceIndexCacheSize = 8192;

This value needs to be a power of 2 for the cache to work. Can you add a static (or runtime) assert that this value is a power of 2 please?
Attachment #8927691 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/f5becc2dcc8e
https://hg.mozilla.org/mozilla-central/rev/7f26ac4150e5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
having the resampling rate made constant was an error :(

https://hg.mozilla.org/mozilla-central/rev/7f26ac4150e5#l7.13

there's no point for having any resampling features if that pref got removed...that includes media.resampling.enabled it only makes sense in combination of the actual sampling rate

this allowed to exercise various code path, otherwise not used.

Just last months a bug was found thanks to playing with those two prefs which easily allow to simulate audio cards we don't have.

I'll re-add those two prefs.
Depends on: 1417083
Also find the move to MediaDecoderLimitDefault in MediaFormatReader disappointing. We've always managed to have MediaFormatReader totally platform agnostic, and this ugly change made it not so
You need to log in before you can comment on or make changes to this bug.