Closed Bug 1206637 Opened 4 years ago Closed 3 years ago

Implement non-mainthread preference for media

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: alwu, Assigned: jya)

References

Details

Attachments

(2 files)

* 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
See Also: → 1175447
Component: Audio/Video → Audio/Video: Playback
Assignee: nobody → jyavenard
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/
Attachment #8750555 - Flags: review?(cpearce)
Attachment #8750556 - Flags: review?(cpearce)
Blocks: 1271491
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 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/
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 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.
Attachment #8750555 - Flags: review?(cpearce) → review+
Attachment #8750556 - Flags: review?(cpearce) → review+
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
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?
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
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 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 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 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 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 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/
https://hg.mozilla.org/mozilla-central/rev/ac4e5703ea9a
https://hg.mozilla.org/mozilla-central/rev/04331c8f60b6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1273945
You need to log in before you can comment on or make changes to this bug.