Closed Bug 910860 Opened 6 years ago Closed 6 years ago

PNG decoder gets preferences off the main thread

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: khuey, Assigned: milan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

7.35 KB, patch
milan
: review+
Details | Diff | Splinter Review
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
Kyle, is there a good way for me to push this to try with the changes needed to catch the problems?
Assignee: nobody → milan
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.)
Attachment #797491 - Flags: review?(seth) → review-
(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.
The fix to bug 912794 should take care of this.
Ping?
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
After bug 912794 lands, will ask for review.
Attachment #797491 - Attachment is obsolete: true
Will take care of the "only evaluate once" in bug 971126.
Attachment #8373795 - Attachment is obsolete: true
Attachment #8376291 - Flags: review?(bgirard)
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
Attachment #8376291 - Flags: review?(bgirard) → review+
(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.
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.
Attachment #8376291 - Attachment is obsolete: true
Hmm, did in Comment 15 and only rebased to swap two different bugs around, but either way, guess didn't do it right.
Oh, I see, bug 972099 got backed out and I didn't notice.  Add a direct dependency because this does depend on it.
Depends on: 972099
No longer depends on: 972099
https://hg.mozilla.org/mozilla-central/rev/1c5922e166f9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.