Open Bug 585074 Opened 11 years ago Updated 2 years ago

Export color profile information using GfxInfo

Categories

(Core :: GFX: Color Management, defect)

x86
Linux
defect
Not set
normal

Tracking

()

People

(Reporter: jrmuizel, Unassigned, Mentored)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: good-first-bug)

Attachments

(4 files)

The should be able to use the new GfxInfo class in widget.
Whiteboard: [good first bug]
Blocks: 491542
Summary: Export color profile information to about:support on linux → Export color profile information using GfxInfo
Duplicate of this bug: 675296
This would be really neat. If someone want to work on this I am willing to mentor.
Whiteboard: [good first bug] → [good first bug][mentor=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
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.
Attached patch Messy WIPSplinter Review
(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.
Benoit , can you  let me know the channel  you  are available in mozilla irc
I'm in both #gfx (great for asking questions about the gfx code) and #developers (for asking general questions about the firefox platform code).
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.
Depends on: 460269
Benoit, I'm on it. First steps?
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?
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)
No rush, just wanted to know if you were still available.
Blocks: 508862
Blocks: 601932
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
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!
Assignee: nobody → almasry.mina
Depends on: 907196
Assignee: malmasry → nobody
Assignee: nobody → bengol2005
is there a UI to watch the exported info already?
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.
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
or a appropriate process flow on this task, it is my simple thought on this above.
Flags: needinfo?(bgirard)
(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);
Flags: needinfo?(bgirard)
Hi guozhu, are you still working on this bug?
Flags: needinfo?(bengol2005)
Unassigning for lack of response.
Assignee: bengol2005 → nobody
Flags: needinfo?(bengol2005)
sorry, I am still working on this, can't check the email because of network issue
Assignee: nobody → bengol2005
Mentor: bgirard
Whiteboard: [good first bug][mentor=benwa] → [good first bug]
Assignee: bengol2005 → nobody
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).
Attachment #8515640 - Flags: review?(bgirard)
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
Attachment #8515640 - Flags: review?(bgirard) → feedback+
> 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.
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
Hi, Cojo - are you still working on this?
Flags: needinfo?(cojojennings)
Probably best to let someone else have a shot. Little out of touch with it now
Flags: needinfo?(cojojennings)
Hi, I'd like to work on this.
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!
Great thanks!  I'm setup already and have started using Benoit's WIP as a baseline.  Benoit are you still available to mentor?
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.
Attachment #8581058 - Flags: review?(jmuizelaar)
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.
Attachment #8581058 - Flags: review?(jmuizelaar)
Attachment #8581058 - Flags: review?(bmcbride)
Attachment #8581058 - Flags: review+
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.
Attachment #8581058 - Flags: review?(bmcbride) → review-
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?
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(jmuizelaar)
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
Thanks for your interest, Albin - take a look at it, and if we can answer any questions for you let us know here.
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.
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.
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.
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
Flags: needinfo?(mhoye)
Flags: needinfo?(bgirard)
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 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.
Flags: needinfo?(bgirard)
Flags: needinfo?(mhoye)
Keywords: good-first-bug
Whiteboard: [good first bug]
You need to log in before you can comment on or make changes to this bug.