1414759 - Streamline MediaPrefs

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

MediaPrefs has some dead prefs and some prefs that could just be code constants.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove dead media prefs

Comment 2

[Nicholas Nethercote [inactive]]

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 :)

Comment 3

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

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.

Comment 4

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

(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.

Comment 5

[Nicholas Nethercote [inactive]]

> 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.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Replace some unnecessary media prefs with code constants

Specifically:
- media.decoder.limit
- media.num-decode-threads
- media.resampling.rate
- media.wmf.decoder.thread-count
- media.cache.resource-index

Comment 7

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

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?

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5becc2dcc8ed9350b7f3a1742276839d2e79dc3
Bug 1414759 - Remove dead media prefs. r=cpearce

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f26ac4150e5bb809de36a24cd5d8bad5a53e9de
Bug 1414759 - Replace some unnecessary media prefs with code constants. r=cpearce

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f5becc2dcc8e
https://hg.mozilla.org/mozilla-central/rev/7f26ac4150e5

Comment 10

[Jean-Yves Avenard [:jya]]

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.

Comment 11

[Jean-Yves Avenard [:jya]]

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