910860 - PNG decoder gets preferences off the main thread

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The PNG decoder calls gfxPlatform::GetRenderingIntent.  Since decoding now happens off the main thread we end up getting preferences there.

http://hg.mozilla.org/mozilla-central/annotate/21b5af569ca2/image/decoders/nsPNGDecoder.cpp#l557

https://tbpl.mozilla.org/php/getParsedLog.php?id=27186173&tree=Try#error0

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Avoid getting CMS prefs on non-main thread.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Kyle, is there a good way for me to push this to try with the changes needed to catch the problems?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You can grab what I pushed at https://hg.mozilla.org/try/rev/487c3c06b3af

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 797491 [details] [diff] [review]
Avoid getting CMS prefs on non-main thread.

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

It looks like this is taking us down the right path, but we're not quite there yet.

::: gfx/thebes/gfxPlatform.cpp
@@ +110,5 @@
> +static bool gCMSPrefEnableV4 = false;
> +static bool gCMSPrefForceSRGB = false;
> +static int32_t gCMSPrefMode = -1;
> +static int32_t gCMSPrefIntent = -1;
> +static nsAdoptingCString gCMSPrefDisplayProfile;

Assigning an nsAdoptingCString isn't atomic on any platform. You need a lock to protect these variables.

I'd also prefer if you moved all of these variables into a class or struct using the singleton pattern. I'm not a big fan of static initializers. (Mostly this is a concern for gCMSPrefDisplayProfile, again, but may as well put them all in there.)

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Seth Fowler [:seth] from comment #4)
> Comment on attachment 797491 [details] [diff] [review]
> Avoid getting CMS prefs on non-main thread.
> 
> Review of attachment 797491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like this is taking us down the right path, but we're not quite
> there yet.
> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +110,5 @@
> > +static bool gCMSPrefEnableV4 = false;
> > +static bool gCMSPrefForceSRGB = false;
> > +static int32_t gCMSPrefMode = -1;
> > +static int32_t gCMSPrefIntent = -1;
> > +static nsAdoptingCString gCMSPrefDisplayProfile;
> 
> Assigning an nsAdoptingCString isn't atomic on any platform. You need a lock
> to protect these variables.

Thanks that makes sense.  I was just doing what the code was doing before, but that was not ready for multiple threads, so it makes sense that doing what was done before isn't enough.

> 
> I'd also prefer if you moved all of these variables into a class or struct
> using the singleton pattern. I'm not a big fan of static initializers.
> (Mostly this is a concern for gCMSPrefDisplayProfile, again, but may as well
> put them all in there.)

I like that.  I wasn't quite sure what pattern to follow in this file, but maybe I'll just go with "what's right" and not worry about matching the rest.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

The fix to bug 912794 should take care of this.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ping?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

The fix to bug 912794 will take care of this, but it's a bit messy fixing that bug.  We can't do lazy evaluation of the preferences anymore, and while standard Firefox initializes gfxPlatform "first", some of the reftests do not.  So, to make tests pass, we need to make the code look ugly.  I'll post something in bug 912794 about this.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to get to the preference on main thread.

After bug 912794 lands, will ask for review.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to get to the preference on main thread.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to make sure preferences are evaluated on the main thread. This still leaves the preference re-evaluated.

Will take care of the "only evaluate once" in bug 971126.

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8376291 [details] [diff] [review]
Use gfxPrefs to make sure preferences are evaluated on the main thread. This still leaves the preference re-evaluated.

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

See my review in https://bugzilla.mozilla.org/show_bug.cgi?id=946907#c5

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from Bug 946907 comment #5)
> Comment on attachment 8375826 [details] [diff] [review]
> ...
> >  
> >  int
> >  gfxPlatform::GetRenderingIntent()
> >  {
> >      if (gCMSIntent == -2) {
> 
> Here -2 appears to mean 'I haven't read the property yet'. This is kind of
> ugly because a user could request -2 but fine because about:config is for
> power users anyways.

This was there before, and I think it's OK.  After the inside of the if is called once (before or now), it is guaranteed not to be -2.

> 
> maybe we should #define QCMS_INTENT_UNITIALIZE -2.

Are you thinking in qcms.h or in this file?

> 
> This code should really initialize the preference to -1 (use embedded
> profile) and validate the range once when reading the preference simplifying
> this logic greatly. But I'm ok to r+ just with the -2 define if you're not
> looking to refactor this here.
>
> 
> @@ +1656,5 @@
> >      if (gCMSIntent == -2) {
> >  
> >          /* Try to query the pref system for a rendering intent. */
> > +        int32_t pIntent = gfxPrefs::CMSRenderingIntent();
> > +        if (pIntent != -2) {
> 
> I don't think this 'if' here is needed. If it's not there line 1662 will
> reject -2 and we will initialize gCMSIntent to -1 and wont evaluate 1656 as
> true in the future.
> 

Just to look at it all together.

Before the change:

If the preference is not set, GetRenderingIntent would return QCMS_INTENT_DEFAULT (which is 0).
If the preference is set to a value between 0 and 3, GetRenderingIntent would return the preference value.
If the preference is set outside of the range [0,3], GetRenderingIntent would return -1.

After the change:
If the preference is not set, gCMSIntent GetRenderingIntent would return QCMS_INTENT_DEFAULT (which is 0).
If the preference is set to a value between 0 and 3, GetRenderingIntent would return the preference value.
** If the preference is set to -2, gCMSIntent GetRenderingIntent would return QCMS_INTENT_DEFAULT (which is 0).
If the preference is outside of the range [0,3] and not -2, gCMSIntent GetRenderingIntent would return -1.

As you pointed out, the behavior ** where the user sets the preference to -2 is different than before.


However, if we get rid of the "if: you mentioned, I think we would get:

** If the preference is not set, gCMSIntent GetRenderingIntent would return -1.
If the preference is set to a value between 0 and 3, GetRenderingIntent would return the preference value.
If the preference is outside of the range [0,3] and not -2, gCMSIntent GetRenderingIntent would return -1.

So, in this case, we're changing the default behavior (preference not set) to go from default to embedded. Is that OK?  If it is, it's probably a separate bug, because, well, the default is changing.

I could be reading the code wrong.


> ::: gfx/thebes/gfxPrefs.h
> @@ +102,5 @@
> >    // This is where DECL_GFX_PREFS for each of the preferences should go.
> >    // We will keep these in an alphabetical order to make it easier to see if
> >    // a method accessing a pref already exists. Just add yours in the list.
> >  
> > +  DECL_GFX_PREFS(Live, "gfx.color_management.enablev4",         CMSEnableV4, bool, false);
> 
> Are you planning on later changing these to be Once? For example changing
> this pref in a running session will ignore CLUT tags from the output profile
> and current images but will honor them in any new images which will lead to
> inconsistent results.

Yup, this is bug 971126.
> 
> @@ +103,5 @@
> >    // We will keep these in an alphabetical order to make it easier to see if
> >    // a method accessing a pref already exists. Just add yours in the list.
> >  
> > +  DECL_GFX_PREFS(Live, "gfx.color_management.enablev4",         CMSEnableV4, bool, false);
> > +  DECL_GFX_PREFS(Live, "gfx.color_management.mode",             CMSMode, int32_t,- 1);
> 
> nit: -1
Indeed, thanks.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Of course, a wee bit of refactoring would simplify this if we just set the default preference to 0.  Let me put a patch together with that.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to make sure preferences are evaluated on the main thread. This still leaves the preference re-evaluated. Carry r=bgirard

https://tbpl.mozilla.org/?tree=Try&rev=cedb8d27a15a

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to make sure preferences are evaluated on the main thread. This still leaves the preference re-evaluated. Carry r=bgirard

Rebasing.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18183f6ef0d8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell and reftest failures. Please run this through Try before requesting checkin next time.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab5b318dcfc

https://tbpl.mozilla.org/php/getParsedLog.php?id=35054050&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35052378&tree=Mozilla-Inbound

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Hmm, did in Comment 15 and only rebased to swap two different bugs around, but either way, guess didn't do it right.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Oh, I see, bug 972099 got backed out and I didn't notice.  Add a direct dependency because this does depend on it.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to make sure preferences are evaluated on the main thread. This still leaves the preference re-evaluated. Carry r=bgirard

https://tbpl.mozilla.org/?tree=Try&rev=336948a9bcba

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5922e166f9

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c5922e166f9