585074 - Export color profile information using GfxInfo

Status: NEW

(Core :: Graphics: Color Management, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

The should be able to use the new GfxInfo class in widget.

Comment 2

[Benoit Girard (:BenWa)]

This would be really neat. If someone want to work on this I am willing to mentor.

Comment 3

[Benoit Girard (:BenWa)]

To give a bit of context for people who would like to work on this bug:

Images can be stored in a specific color space described by an ICC color profile. Color profiles describe the transformation from the image's color space to the monitor's color space.

By exporting this information with GfxInfo information about the image/monitor color profile (color space) can be made available to components of the browser such as about:support and extensions. This is particularly useful for professional, debugging and testing. 

Relevant area in the code base:
http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/GfxInfoBase.h#54
http://mxr.mozilla.org/mozilla-central/ident?i=GfxInfo&tree=mozilla-central&filter=

qcms interface:
http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/qcmsint.h
http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/qcms.h

Comment 4

[Sudheendra]

Hi  Benoit , 
I am interested to  work  on this bug.  I have gone through the docs u  have suggested. 
Can you  please let me know whats  next step  in fixing  this.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Messy WIP

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Sudheendra from comment #4)
> Hi  Benoit , 
> I am interested to  work  on this bug.  I have gone through the docs u  have
> suggested. 
> Can you  please let me know whats  next step  in fixing  this.

Hello Sudheendra,

I have already did a bit of hacking on this but don't plan on finishing it any time soon. You can start by doing a build of firefox for your platform without the patch applied since I think it doesn't compile. Once that is working you can either apply my patch or start from scratch.

In my patch I was planning on returning a Base64 DataURI of the monitor profile. This is to add a 'Download' button to the about:support page for users to upload their color profile when filling bug reports. You will most likely want to redo the method body. I was playing around with the best way to read in the platform's specific color profile but I'm not convinced if adding a |path| member to the qcms profile is the way to go.

Let me know if you have any questions with hg, compiling or this patch in specific. You can also find me on the mozilla irc as BenWa.

Comment 7

[Sudheendra]

Benoit , can you  let me know the channel  you  are available in mozilla irc

Comment 8

[Benoit Girard (:BenWa)]

I'm in both #gfx (great for asking questions about the gfx code) and #developers (for asking general questions about the firefox platform code).

Comment 9

[Jeff Muizelaar [:jrmuizel]]

on X11 we get the profiles from the xserver and not from files, so doing something specific to files is out of the question. Here's my proposal:

Add a function like GfxPlatform::GetProfile(const void *&mem, size_t &size);
This returns a pointer to the memory for this profile.

Add a function like qcms_profile_from_path_to_mem(const char *path, void **mem, size_t **size);
That reads the profile into memory.

Reorganize the existing functionality on top of this.

Hopefully, that's clear enough.

Comment 10

[Yati Sagade]

Benoit, I'm on it. First steps?

Comment 11

[Benoit Girard (:BenWa)]

Hello Yati, we talked over email in November. Are you still interested in looking into this issue or should we let someone else have a chance?

Comment 12

[Yati Sagade]

I am in, Benoit - was just a bit swamped. I'll catch up with you on #gfx first thing tomorrow(it's 3 am here now)

Comment 13

[Benoit Girard (:BenWa)]

No rush, just wanted to know if you were still available.

Comment 14

[Mina Almasry]

Hi I'm new here and I'd like to take a shot at this. Can I get assigned? (or is that not how this works?)

Thanks

Comment 15

[Mina Almasry]

I'll take a shot at this, if that's okay. I'll be back with questions or ping you irc if I have any. Thanks!

Comment 16

[guozhu cheng]

is there a UI to watch the exported info already?

Comment 17

[Benoit Girard (:BenWa)]

The goal is to export the information in the page 'about:support' (Also in help->Troubleshooting). With the patch from bug 907196 we should be able to provide a download link where external ICC profile viewer can be used and users can upload their profiles on bugzilla for help.

For fun later we could export more information about the profile transformation themselves or move to improving the ICCv4 support so that it can be turned on by default.

Comment 18

[guozhu cheng]

hi benwa, since the bug 907196 have been fixed, I start to implement this task. and my idea is:

(1) add a button near "Copy text to Clipboard" named "export ICC profile" (have done)
(2) when click, open a platform-depended file svaing dialog with a default filename to select a user choose file location.(working on this, need to know how to get a user choose file path)
(3) send this variable to the core (somewhere ? need help) to save the ICC profile to the disk. 

can you give some advice (function module, file name, or website) on (2) and (3) ? thank you very much

Comment 19

[guozhu cheng]

or a appropriate process flow on this task, it is my simple thought on this above.

Comment 20

[Benoit Girard (:BenWa)]

(In reply to guozhu cheng from comment #18)
> hi benwa, since the bug 907196 have been fixed, I start to implement this
> task. and my idea is:
> 
> (1) add a button near "Copy text to Clipboard" named "export ICC profile"
> (have done)
Actually under the 'Graphics' section can you instead add a heading there: 'Display ICC Profile:'
> (2) when click, open a platform-depended file svaing dialog with a default
> filename to select a user choose file location.(working on this, need to
> know how to get a user choose file path)
> (3) send this variable to the core (somewhere ? need help) to save the ICC
> profile to the disk. 
> 
> can you give some advice (function module, file name, or website) on (2) and
> (3) ? thank you very much

Ideally you can keep modifying gfxInfoBase to return a data URI string and have the export button save the string.

Once you have the string you can initialize a download (save) using this snippet:
    var blob = new Blob([OUTPUT_PROFILE], { "type": "application/octet-stream" });
     location.href = window.URL.createObjectURL(blob);

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Hi guozhu, are you still working on this bug?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Unassigning for lack of response.

Comment 23

[guozhu cheng]

sorry, I am still working on this, can't check the email because of network issue

Comment 24

[Connor [:cojo]]

Attachment: 585074-export-color-profile.diff

This is an updated version of Benoit's patch. I took advantage of bug 907196 and utilized |GetCMSOutputProfile| to get the profile data.

I moved the construction of the base64 data uri to the frontend since it seemed like there could be other circumstances where getting just the profile data would be useful instead of a profile data uri (not sure if that's a valid assumption, so please correct me if I'm wrong).

Comment 25

[Benoit Girard (:BenWa)]

Comment on attachment 8515640 [details] [diff] [review]
585074-export-color-profile.diff

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

Looks good, just need to fix the encoding. Let me know if you think it's correct.

::: widget/GfxInfoBase.cpp
@@ +830,5 @@
> +  qcms_profile* profile = nullptr;
> +
> +  profile = gfxPlatform::GetCMSOutputProfile();
> +
> +  aResult.AppendPrintf("%s",

Is this correct? I don't think you can format binary data as a string. I think you need to base64 encode here.

Maybe you need to use PL_Base64Encode:
http://mxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/include/plbase64.h#35

Comment 26

[Connor [:cojo]]

> Is this correct? I don't think you can format binary data as a string. I think you need to base64 encode here.

Good question, not sure. I'll investigate using PL_Base64Encode.

Comment 27

[Connor [:cojo]]

Think I was a bit confused.

> gfxPlatform::GetCMSOutputProfile();
This returns a qcms_profile*. In your patch [1] you had the following:
> file = fopen(outputProfile->path, "rb");

Which I think was meant to be used to read the underlying display profile data.

Is there a way to encode qcms_profile* so that it can be passed to the frontend? Doesn't seem like PL_Base64Encode solves that problem. If not, is there a way to find the binary data of a qcms_profile*? void gfxPlatform::GetCMSOutputProfileData seems like it would be useful but it's a private method.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=551454&action=diff#a/widget/src/xpwidgets/GfxInfoBase.cpp_sec3

Comment 28

[Mike Hoye [:mhoye]]

Hi, Cojo - are you still working on this?

Comment 29

[Connor [:cojo]]

Probably best to let someone else have a shot. Little out of touch with it now

Comment 30

[Derek]

Hi, I'd like to work on this.

Comment 31

[Mike Hoye [:mhoye]]

No time like the present, Derek! Thanks for your interest.

There's some good information here about how to get your development environment set up:

https://developer.mozilla.org/en-US/docs/Introduction

and if you have any questions, feel free to join us on IRC in the #introduction channel, or ask in this bug. Thanks, and good luck!

Comment 32

[Derek]

Great thanks!  I'm setup already and have started using Benoit's WIP as a baseline.  Benoit are you still available to mentor?

Comment 33

[Derek]

Attachment: Download color profile from about:support page

Comment 34

[Derek]

Comment on attachment 8581058 [details] [diff] [review]
Download color profile from about:support page

I've used Benoit's and Connor's work as a baseline and worked from there.  I've tested this on osx.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8581058 [details] [diff] [review]
Download color profile from about:support page

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

Unfocused should look at the aboutSupport changes. The Gfx parts seems to fine.

Comment 36

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8581058 [details] [diff] [review]
Download color profile from about:support page

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

I wasn't able to build this to test it - presumably because it seems to be based on quite an old tree.

::: toolkit/content/aboutSupport.js
@@ +629,5 @@
> +function downloadCmsProfile(button) {
> +  var gfxInfo = null;
> +
> +  try {
> +    gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);

If nsIGfxInfo isn't available, the button to download the color profile shouldn't be shown at all.

@@ +636,5 @@
> +  if (gfxInfo) {
> +    let cmsProfileData = gfxInfo.getCmsMonitorProfileData();
> +    let dataURI = ["data:vnd/iccprofile;" +
> +                   "base64," +
> +                   cmsProfileData];

This should just be a string, not an array containing a string. ie:

  let dataURI = "data:vnd/iccprofile;base64," + cmsProfileData;

@@ +671,5 @@
>    $("profile-dir-button").addEventListener("click", function (event){
>      openProfileDirectory();
>    });
> +  $("download-cms-profile-button").addEventListener("click", function (event){
> +    downloadCmsProfile(this);

Don't need to pass in the button to this - it's not used.

::: toolkit/content/aboutSupport.xhtml
@@ +251,5 @@
> +        &aboutSupport.cmsTitle;
> +      </h2>
> +
> +      <table>
> +        <tbody id="cms-tbody">

There isn't any useful information here that we'd want included when someone clicks the "Copy test to clipboard" button. Use the "no-copy" class to avoid having this table added to the clipboard.

Comment 37

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I guess there's the question of whether the color profile data should be included in the raw data. ie, is the profile data useful for diagnostic bug reports, etc? At the moment it isn't included - it involves a separate step for someone to save and upload separately.

I don't have any context to answer that, nor any context to know what size data this could be (Few Kb? Is a couple of Mb possible?). Jeff?

Comment 38

[Jeff Muizelaar [:jrmuizel]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #37)
> I guess there's the question of whether the color profile data should be
> included in the raw data. ie, is the profile data useful for diagnostic bug
> reports, etc? At the moment it isn't included - it involves a separate step
> for someone to save and upload separately.
> 
> I don't have any context to answer that, nor any context to know what size
> data this could be (Few Kb? Is a couple of Mb possible?). Jeff?

That separate step is fine. The profile could be largeish (1-2 Mb) and isn't usually useful.

Comment 39

[albinkun]

Hi All,

I'd like to work on this one as my first bug, I'll setup my environment and then ask for advice :). My read on this is that it needs to be refreshed to the current source code and make a couple of changes.

Cheers,
Albin

Comment 40

[Mike Hoye [:mhoye]]

Thanks for your interest, Albin - take a look at it, and if we can answer any questions for you let us know here.

Comment 41

[albin]

Hi all again, 

Sorry for the late update. Just wanted to touch base, I am now posting from my 'serious' account and built Firefox in Linux. I ported all of the previous code that was in the previous attempts to the current tree, but I still have to make the changes requested by Blair, just gotta brush up real quick on my JS knowledge and will do that. 

The code seems to work, clicking the button triggers a download of what I assumed was a color profile. I installed a color profile viewer in Ubuntu and it in fact recognizes it as the ICC profile I am currently using. 

I will implement the changes suggested by Blair and then submit the patch for review.

Comment 42

[albin]

Attachment: albihern-585074.patch

I tried joining the #gfx IRC channel to ask some questions but all my messages got blocked :) am I doing IRC wrong? I registered the nick: albihern 

But I always get: Message to channel blocked and opers notified (hibot)

Anyways, I was just wondering how to address this specific comment from Blair: 

If nsIGfxInfo isn't available, the button to download the color profile shouldn't be shown at all.

Should I pass the button so I can disable/hide it in the JS function?

Some unrelated changes are in the patch as well. Not sure what happened there. Sorry about that, first time using mercurial.

Comment 43

[albin]

Ah, seems like I erased some whitespace with some of my emacs defaults. Sorry, I can re-do the patch without these changes if they are in bad taste. Sorry for the spam.

Comment 44

[albin]

Just wondering if you could have a look at the patch, it's still a bit rough but wanted your input before further working on it. 

Thanks, Albin

Comment 45

[Benoit Girard (:BenWa)]

Took an brief look. Patch looks good. I'll review it tomorrow.

I'm not sure what the IRC issue is, perhaps mhoye can help.

> first time using mercurial.

If you prefer you can use GIT and the mirror https://github.com/mozilla/gecko-dev. A lot of people are using GIT so even if it's not the official source control you wont hit any major issues.

Comment 46

[Benoit Girard (:BenWa)]

Comment on attachment 8767379 [details] [diff] [review]
albihern-585074.patch

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

Looks good, I think once these changes are made the patch should be ready to land. Thank you!

::: gfx/thebes/gfxPlatform.h
@@ +485,5 @@
>  
>      /**
> +     * Return the unaltered output device ICC profile.
> +     */
> +    static void GetRawCMSOutputProfile(void *&mem, size_t &size);

Please use pointers instead of by reference here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#COM_and_pointers

This will make the call site better because it's not clear if you're modifying mem, size or both.

Please use aFoo for the parameters.

Also you're leaking mem at the callsite. Let's fix this by using UniquePtr<void*> instead of void*.

::: widget/GfxInfoBase.cpp
@@ +933,5 @@
> +
> +    const char* base64Data = PL_Base64Encode((const char*) profile, size, nullptr);
> +    aResult = NS_ConvertASCIItoUTF16(base64Data);
> +
> +    NS_Free((void*) base64Data);

I think you need the void* there because you're not const correct. You really have a char* and not a const char*.

::: widget/nsIGfxInfo.idl
@@ +17,5 @@
>    readonly attribute boolean DWriteEnabled;
>    readonly attribute DOMString DWriteVersion;
>    readonly attribute DOMString cleartypeParameters;
>  
> +  // XXX: Switch to a list of devices, rather than explicitly numbering them.

It's a bit annoying but please avoiding unrelated whitespace changes. It causes headaches with unrelated conflicts and makes backout and uplift needlessly complicated.