Closed Bug 907196 Opened 11 years ago Closed 10 years ago

Split CreateCMSOutputProfile profile loading into GetCMSOutputProfileData

Categories

(Core :: Graphics: Color Management, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: BenWa, Assigned: bengol2005)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=benwa])

Attachments

(1 file, 7 obsolete files)

+++ This bug was initially created as a clone of Bug #585074 +++

In bug 585074 Comment 9 Jeff points out that it's not easy to get the display profile across all platforms.

In this bug we need to refactor 'gfxPlatform::CreateCMSOutputProfile()'. Currently this function locates and opens the profile. Instead we need to add an intermediate function 'GfxPlatform::GetCMSOutputProfileData(const void *&mem, size_t &size)' which takes the first half of the logic from gfxPlatform::CreateCMSOutputProfile() but returns a pointer to the memory to load this profile. gfxPlatform::CreateCMSOutputProfile() should then call GfxPlatform::GetProfile(const void *&mem, size_t &size) instead of duplicating this logic and use qcms_profile_from_memory.
Assignee: nobody → almasry.mina
Attached patch Patch (obsolete) — Splinter Review
Is this what you want? I ended up having to make more changes than I thought I would have to, to keep track of the size of the profile in functions called from GetPlatformCMSOutputProfile

try push: https://tbpl.mozilla.org/?tree=Try&rev=0c8fc1f234bd
Attachment #792908 - Flags: review?(bgirard)
Comment on attachment 792908 [details] [diff] [review]
Patch

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

This patch is on the right track and most of the way there I think. But it looks like you misunderstood the binary ICC format which is described here:
http://www.color.org/specification/ICC1v43_2010-12.pdf

and should be typed const char* + size_t size vs. qcms_profile* which is a deserialized parsed version of that format.

::: gfx/qcms/qcms.h
@@ +136,5 @@
>  
>  qcms_profile* qcms_profile_from_memory(const void *mem, size_t size);
>  
> +qcms_profile* qcms_profile_from_file(FILE *file, size_t *size);
> +qcms_profile* qcms_profile_from_path(const char *path, size_t *size);

We don't need size here.

::: gfx/thebes/gfxPlatform.cpp
@@ +1575,5 @@
> +            }
> +        }
> +    }
> +
> +    mem = gfxPlatform::GetPlatform()->GetPlatformCMSOutputProfile(size);

This returns qcms_profile* which is not the same as a binary ICC profile.

@@ +1597,5 @@
> +        const void * mem;
> +        size_t size;
> +        GetCMSOutputProfileData(mem, size);
> +        if (mem && size > 0) {
> +          qcms_profile_from_memory(mem, size);

gCMSOutputProfile = qcms_profile_from_memory(mem, size);
mem needs to be free-ed

::: gfx/thebes/gfxPlatformMac.h
@@ +84,5 @@
>  
>      virtual already_AddRefed<gfxASurface>
>      CreateThebesSurfaceAliasForDrawTarget_hack(mozilla::gfx::DrawTarget *aTarget);
>  private:
> +    virtual qcms_profile* GetPlatformCMSOutputProfile(size_t &size);

This should be modified to return a binary ICC profile instead of opening it directly.

::: gfx/thebes/gfxWindowsPlatform.h
@@ +309,5 @@
>      mozilla::RefPtr<ID3D11Device> mD3D11Device;
>      bool mD3D9DeviceInitialized;
>      bool mD3D11DeviceInitialized;
>  
> +    virtual qcms_profile* GetPlatformCMSOutputProfile(size_t& size);

This as well.
Attachment #792908 - Flags: review?(bgirard) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Right I did misunderstand what was needed. Here is a patch with the changes.

The only place I did not know what to do was when qcms_profile_create_rgb_with_gamma was called. That function just creates a profile and is called from gfxPlatformGtk::GetPlatformCMSOutputProfile

Here is an updated patch. It builds on mac, but I'm not sure it's correct for windows and gtk :(
Attachment #792908 - Attachment is obsolete: true
Attachment #793170 - Flags: review?(bgirard)
Attached patch Patch v2 (obsolete) — Splinter Review
Sorry this is the updated patch.
Attachment #793170 - Attachment is obsolete: true
Attachment #793170 - Flags: review?(bgirard)
Attachment #793171 - Flags: review?(bgirard)
Comment on attachment 793171 [details] [diff] [review]
Patch v2

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

::: gfx/qcms/qcms.h
@@ +135,5 @@
>  		float gamma);
>  
>  qcms_profile* qcms_profile_from_memory(const void *mem, size_t size);
>  
> +void* qcms_data_from_file(FILE *file, size_t* size);

We should be leaving these function intact. There are other users of this API like our unit tests (and Chrome?) and we should keep it stable. We want to continue supporting loading from a FILE*->qcms_profile*. A helper function to read a file into a void* doesn't belong in qcms.

::: gfx/thebes/gfxPlatform.cpp
@@ +1563,5 @@
> +        if (file) {
> +            fseek(file, 0, SEEK_END);
> +            size_t length = ftell(file);
> +            fseek(file, 0, SEEK_SET);
> +            void * data = malloc(length * sizeof(char));

Let's check for data being null. If gfx.color_management.display_profile points to a large file this allocation can reasonably fail.

@@ +1567,5 @@
> +            void * data = malloc(length * sizeof(char));
> +            size_t result = fread(data, length, 1, file);
> +            fclose(file);
> +
> +            if (result == length) {

&& size > 0

@@ +1571,5 @@
> +            if (result == length) {
> +                mem = data;
> +                size = length;
> +                return;
> +            }

else {
  free(data)
}

@@ +1595,5 @@
>          }
>  
> +        void* mem;
> +        size_t size;
> +        GetCMSOutputProfileData(mem, size);

You need to keep the !gCMSOutputProfile around this such that force_srgb continues to work.

::: gfx/thebes/gfxPlatformGtk.cpp
@@ -548,2 @@
>  
> -            XFree(retProperty);

I'm not familiar with GTK but I'm going to guess that XFree will invalidate retProperty which we will be trying to return here. I think the code you have will leak the property. We should instead malloc a buffer of retLength and memcpy regProperty there.

@@ +616,5 @@
>                      primaries.Blue.x, primaries.Blue.y, primaries.Blue.Y);
>  #endif
>  
> +            void* data =
> +                qcms_data_create_rgb_with_gamma(whitePoint, primaries, gamma);

As you pointed out this will be a problem. We can't return this as a data void* because it's a qcms_profile.

Let's ask Jeff if he has any suggestions here.

::: gfx/thebes/gfxPlatformMac.cpp
@@ +492,5 @@
>          FSRef fsRef;
>          if (!FSpMakeFSRef(&location->u.fileLoc.spec, &fsRef)) {
>              char path[512];
>              if (!FSRefMakePath(&fsRef, reinterpret_cast<UInt8*>(path), sizeof(path))) {
> +                data = qcms_data_from_path(path, &length);

This will need to open the file itself.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1107,3 @@
>          return nullptr;
>  
> +    void* data = qcms_data_from_unicode_path(str, &size);

and this to.
Attachment #793171 - Flags: review?(bgirard) → feedback+
Flags: needinfo?(jmuizelaar)
Jeff just a friendly reminder this is still waiting for that need info flag. Thanks!
Un-Assigning and removing the needinfo flag because I won't be able to work on this. But the needinfo might be useful for the next person that works on this...
Assignee: malmasry → nobody
Flags: needinfo?(jmuizelaar)
Assignee: nobody → bengol2005
I would like to take Mina Almasry 's work and continue fixing this bug, also with bug 585074
have followed && fixed the review previously, but "qcms_data_create_rgb_with_gamma" havn't defined and this can't compile on Linux, will take some time
follow Mina Almasry 's work and the latest review, finish the ICC profile construct on Linux platform
Attachment #824442 - Flags: review?(bgirard)
Attachment #824442 - Attachment is obsolete: true
Attachment #824442 - Flags: review?(bgirard)
Attachment #824453 - Flags: review?(bgirard)
Hello guozhu, I'm away this week and wont be able to review this. I'll happily review this next week. Otherwise you can request jrmuizel to review this patch if you'd like feedback this week.
As this blocks Bug 585074 ,  I will wait for your back to review this next week and give some advice on that bug meanwhile. 
(In reply to Benoit Girard (:BenWa) from comment #12)
> Hello guozhu, I'm away this week and wont be able to review this. I'll
> happily review this next week. Otherwise you can request jrmuizel to review
> this patch if you'd like feedback this week.
Comment on attachment 824453 [details] [diff] [review]
bug-907196-fix.patch v2 : fix a obvious error in previous patch

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

Great work so far! I'm not done the review yet but though I would give you feedback now.

::: gfx/qcms/iccread.c
@@ +141,5 @@
>  
> +static void write_u32(void *mem, size_t offset, uint32_t value)
> +{
> +	be32 k = cpu_to_be32(value);
> +	memcpy(((unsigned char*)mem + offset), &k, sizeof(k));

I don't think it's worth using a memcpy for a single fixed 32bit write. An assignment should be sufficient here.

@@ +147,5 @@
> +
> +static void write_u16(void *mem, size_t offset, uint16_t value)
> +{
> +	be16 k = cpu_to_be16(value);
> +	memcpy(((unsigned char*)mem + offset), &k, sizeof(k));

likewise

@@ +1269,4 @@
>  #endif
> +
> +/*
> +* This function onstructs an ICC profile memory with given header and tag data, which can be read via qcms_profile_from_memory().

constructs*

@@ +1276,5 @@
> +* To construct a valid ICC profile, its divided into three steps :
> +*	(1) construct the r/g/bXYZ part
> +*	(2) construct the r/g/bTRC part
> +*	(3) construct the profile header
> +* this is a hardcode step just for "create_rgb_with_gamma", it is the only requirement till now, maybe we can improve this method

s/improve/make

@@ +1277,5 @@
> +*	(1) construct the r/g/bXYZ part
> +*	(2) construct the r/g/bTRC part
> +*	(3) construct the profile header
> +* this is a hardcode step just for "create_rgb_with_gamma", it is the only requirement till now, maybe we can improve this method
> +* more general in future, but I havn't devices or environment for test and experiment till now.

but I don't have the devices or environment for testing.*

@@ +1279,5 @@
> +*	(3) construct the profile header
> +* this is a hardcode step just for "create_rgb_with_gamma", it is the only requirement till now, maybe we can improve this method
> +* more general in future, but I havn't devices or environment for test and experiment till now.
> +*
> +* NOTE : some of the parameters below are hardcode, to understand it, please refer to "ICC1v43_2010_12.pdf" via Internet, or other

please refer to the ICC documentation.*

@@ +1356,5 @@
> +	}
> +
> +	// Part3 : write profile header
> +	write_u32(data, 0, length); // the total length of this memory
> +	write_u32(data, 12, DISPLAY_DEVICE_PROFILE); // profiler->class

profiler/profile*
Comment on attachment 824453 [details] [diff] [review]
bug-907196-fix.patch v2 : fix a obvious error in previous patch

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

Looks great, great job with the ICCv4 spec! With these fixed the patch should be good to go.

::: gfx/qcms/iccread.c
@@ +1320,5 @@
> +		return;
> +	}
> +
> +	tag_table_offset = ICC_PROFILE_HEADER_LENGTH + 4; // the position of first tag's signature in tag table 
> +	tag_data_offset = ICC_PROFILE_HEADER_LENGTH + 4 + (12 * (xyz_count + trc_count)); // the start of tag data elements.

I'm not strict on the 80 char limit but this is far too long. Lets wrap this.

@@ +1330,5 @@
> +		write_u32(data, tag_table_offset+8, 20); // 20 bytes per TAG_(r/g/b)XYZ tag element
> +
> +		// tag data element
> +		write_u32(data, tag_data_offset, XYZ_TYPE);
> +		write_u32(data, tag_data_offset+8, double_to_s15Fixed16Number(colorants.m[0][index]));

// Reserved 4 bytes

@@ +1335,5 @@
> +		write_u32(data, tag_data_offset+12, double_to_s15Fixed16Number(colorants.m[1][index]));
> +		write_u32(data, tag_data_offset+16, double_to_s15Fixed16Number(colorants.m[2][index]));
> +
> +		tag_table_offset += 12;
> +		tag_data_offset += 20;

Does this need to be a multiple of 4? If so don't forget to update the profile length

@@ +1347,5 @@
> +		write_u32(data, tag_table_offset+8, 14); // 14 bytes per TAG_(r/g/b)TRC element
> +
> +		// tag data element
> +		write_u32(data, tag_data_offset, CURVE_TYPE);
> +		write_u32(data, tag_data_offset+8, 1); // count

I got confused with the size computation until I realized that there's 4 reserved bytes here. Please add:
// Reserved 4 bytes

@@ +1356,5 @@
> +	}
> +
> +	// Part3 : write profile header
> +	write_u32(data, 0, length); // the total length of this memory
> +	write_u32(data, 12, DISPLAY_DEVICE_PROFILE); // profiler->class

Maybe make a note:
// Important header fields are left empty. This generates a profile for internal use only. We should be generating: Profile version (04300000h), Profile signature (acsp), PCS illumiant field. Likewise mandatory profile tags are omitted.

For our purposes this is fine.
Attachment #824453 - Flags: review?(bgirard) → feedback+
Benwa, Did you mean to give me a reivew+ :) ?
and I will fix these issues tomorrow.

BTW, can you have a look at bug 585074, I have a question there.
Attached patch bug-907196-fix-3.patch (obsolete) — Splinter Review
Attachment #828558 - Flags: review?(bgirard)
Attachment #793171 - Attachment is obsolete: true
Attachment #824453 - Attachment is obsolete: true
Comment on attachment 828558 [details] [diff] [review]
bug-907196-fix-3.patch

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

This looks good to me. Lets have Jeff look it over first. Great work!
Attachment #828558 - Flags: review?(jmuizelaar)
Attachment #828558 - Flags: review?(bgirard)
Attachment #828558 - Flags: review+
what should I do next ? 5 day passed and there is no updated on this.
Comment on attachment 828558 [details] [diff] [review]
bug-907196-fix-3.patch

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

Sorry about the delay in the review. This looks pretty good.

::: gfx/qcms/iccread.c
@@ +1164,5 @@
>  }
>  
>  
>  #include <stdio.h>
> +static void qcms_data_frome_file(FILE *file, void **mem, size_t *size)

use 'from' instead of 'frome'
Attachment #828558 - Flags: review?(jmuizelaar) → review+
Attached patch bug-907196-fix-4.patch (obsolete) — Splinter Review
fix the previous review issues, and renew to make it apply to trunk
Attachment #828558 - Attachment is obsolete: true
Keywords: checkin-needed
sorry but I will fix this issue as soon as possible.
fix the compile error on windows platform
Attachment #8358786 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7616c2c9ddee
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.