1206637 - Implement non-mainthread preference for media

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

* Fork from bug1175447 comment29 *

In Gecko, we can't use the preference API in non-mainthread, because it's not thread-safe.

The gfx implements non-mainthread api like [1]. We should also create one for media.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1206637: P1. Add MediaPrefs convenience class. r?cpearce

Almost identical to gfxPrefs, with the exception that preferences can't be set (as it doesn't work with e10s anyway). The generated code size is tiny enough that we don't have to bother about having duplicates.

Review commit: https://reviewboard.mozilla.org/r/51467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51467/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1206637: P2. Replace all cached preferences with MediaPrefs ones. r?cpearce

Additionally, clean up stray and unused Preferences.h header.

Review commit: https://reviewboard.mozilla.org/r/51469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51469/

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750555 [details]
MozReview Request: Bug 1206637: P1. Add MediaPrefs convenience class. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51467/diff/1-2/

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750556 [details]
MozReview Request: Bug 1206637: P2. Replace all cached preferences with MediaPrefs ones. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51469/diff/1-2/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard.mozilla.org/r/51467/#review48663

::: dom/media/MediaPrefs.h:44
(Diff revision 2)
> +class MediaPrefs final
> +{
> +
> +private:
> +  // Since we cannot use const char*, use a function that returns it.
> +  template <class T, T Default(void), const char* Pref(void)>

What is the "void" for?

::: dom/media/MediaPrefs.h:85
(Diff revision 2)
> +      sInstance = new MediaPrefs;
> +    }
> +    MOZ_ASSERT(SingletonExists());
> +    return *sInstance;
> +  }
> +  static void DestroySingleton();

I find this is called nowhere. It might too insignificant to destroy the singleton. So we might just leak it anyway.

::: dom/media/MediaPrefs.h:89
(Diff revision 2)
> +  }
> +  static void DestroySingleton();
> +  static bool SingletonExists();
> +
> +private:
> +  static Atomic<MediaPrefs*> sInstance;

It doesn't have to be an atomic if we can guarantee the 1st call to GetSingleton() is sequenced before all other function calls.

::: dom/media/MediaPrefs.h:89
(Diff revision 2)
> +  }
> +  static void DestroySingleton();
> +  static bool SingletonExists();
> +
> +private:
> +  static Atomic<MediaPrefs*> sInstance;

It doesn't have to be an atomic if we can guarantee the 1st call to GetSingleton() is sequenced before all other function calls.

Comment 6

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

Comment on attachment 8750555 [details]
MozReview Request: Bug 1206637: P1. Add MediaPrefs convenience class. r?cpearce

https://reviewboard.mozilla.org/r/51467/#review49007

::: dom/media/MediaPrefs.h:85
(Diff revision 2)
> +      sInstance = new MediaPrefs;
> +    }
> +    MOZ_ASSERT(SingletonExists());
> +    return *sInstance;
> +  }
> +  static void DestroySingleton();

How about calling MediaPrefs::DestroySingleton() at the same place gfxPrefs::DestroySingleton() is called, in gfxPlatform.cpp:849.

Or, we can have our own startup/shutdown in nsLayoutModule.cpp. I think I'd prefer that, so that we've not got our critical stuff relying on gfx not breaking it.

Comment 7

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

Comment on attachment 8750556 [details]
MozReview Request: Bug 1206637: P2. Replace all cached preferences with MediaPrefs ones. r?cpearce

https://reviewboard.mozilla.org/r/51469/#review49009

Comment 8

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

https://reviewboard.mozilla.org/r/51467/#review49011

::: dom/media/MediaPrefs.h:53
(Diff revision 2)
> +    PrefTemplate()
> +    : mValue(Default())
> +    {
> +      Register(Pref());
> +    }
> +    T mValue;

Technically (and we always got away without doing this, yet jwwang has complained to me about it) if the pref caches aren't Atomic<>, we could end up with the pref changing right before we return it, and returning the wrong value.

I have argued in the past that prefs change rarely enough that we could get away without making pref caches atomic. What do you think?

Comment 9

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/51467/#review49011

> Technically (and we always got away without doing this, yet jwwang has complained to me about it) if the pref caches aren't Atomic<>, we could end up with the pref changing right before we return it, and returning the wrong value.
> 
> I have argued in the past that prefs change rarely enough that we could get away without making pref caches atomic. What do you think?

I guess I felt that there's a difference between reading a wrong pref value and reading the wrong singleton value ; in which case it will crash (either bad pointer or null deref). 

What about I store the singleton in a StaticUniquePtr instead (if such thing exist) that way we don't even have to call DeleteSingleton

Comment 10

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/51467/#review49011

> I guess I felt that there's a difference between reading a wrong pref value and reading the wrong singleton value ; in which case it will crash (either bad pointer or null deref). 
> 
> What about I store the singleton in a StaticUniquePtr instead (if such thing exist) that way we don't even have to call DeleteSingleton

Sorry, I answered a previous question earlier.

Yes, pref caches aren't atomic, but I just don't see how we could do so without extensive rewrite of the pref cache. Typically through the use of callbacks instead that would pass the data by value.

If all pref cache are done via MediaPrefs, then its a change that could be conceived more easily. I will open a follow up bug about it.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750555 [details]
MozReview Request: Bug 1206637: P1. Add MediaPrefs convenience class. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51467/diff/2-3/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750556 [details]
MozReview Request: Bug 1206637: P2. Replace all cached preferences with MediaPrefs ones. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51469/diff/2-3/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750555 [details]
MozReview Request: Bug 1206637: P1. Add MediaPrefs convenience class. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51467/diff/3-4/

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750556 [details]
MozReview Request: Bug 1206637: P2. Replace all cached preferences with MediaPrefs ones. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51469/diff/3-4/

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8750556 [details]
MozReview Request: Bug 1206637: P2. Replace all cached preferences with MediaPrefs ones. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51469/diff/4-5/

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4e5703ea9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/04331c8f60b6

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac4e5703ea9a
https://hg.mozilla.org/mozilla-central/rev/04331c8f60b6