Closed
Bug 907196
Opened 11 years ago
Closed 10 years ago
Split CreateCMSOutputProfile profile loading into GetCMSOutputProfileData
Categories
(Core :: Graphics: Color Management, defect)
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)
26.95 KB,
patch
|
Details | Diff | Splinter Review |
+++ 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.
Updated•11 years ago
|
Assignee: nobody → almasry.mina
Comment 1•11 years ago
|
||
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)
Reporter | ||
Comment 2•11 years ago
|
||
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+
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
Sorry this is the updated patch.
Attachment #793170 -
Attachment is obsolete: true
Attachment #793170 -
Flags: review?(bgirard)
Attachment #793171 -
Flags: review?(bgirard)
Reporter | ||
Comment 5•11 years ago
|
||
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+
Updated•11 years ago
|
Flags: needinfo?(jmuizelaar)
Comment 6•11 years ago
|
||
Jeff just a friendly reminder this is still waiting for that need info flag. Thanks!
Comment 7•11 years ago
|
||
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)
Updated•11 years ago
|
Assignee: nobody → bengol2005
Assignee | ||
Comment 8•11 years ago
|
||
I would like to take Mina Almasry 's work and continue fixing this bug, also with bug 585074
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
follow Mina Almasry 's work and the latest review, finish the ICC profile construct on Linux platform
Attachment #824442 -
Flags: review?(bgirard)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #824442 -
Attachment is obsolete: true
Attachment #824442 -
Flags: review?(bgirard)
Attachment #824453 -
Flags: review?(bgirard)
Reporter | ||
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
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.
Reporter | ||
Comment 14•11 years ago
|
||
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*
Reporter | ||
Comment 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #828558 -
Flags: review?(bgirard)
Reporter | ||
Updated•11 years ago
|
Attachment #793171 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #824453 -
Attachment is obsolete: true
Reporter | ||
Comment 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
what should I do next ? 5 day passed and there is no updated on this.
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
fix the previous review issues, and renew to make it apply to trunk
Attachment #828558 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef14be22d6f7
Keywords: checkin-needed
Comment 23•10 years ago
|
||
Backed out for Windows bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/d9107f528b80 https://tbpl.mozilla.org/php/getParsedLog.php?id=32920474&tree=Mozilla-Inbound
Assignee | ||
Comment 24•10 years ago
|
||
sorry but I will fix this issue as soon as possible.
Assignee | ||
Comment 25•10 years ago
|
||
fix the compile error on windows platform
Attachment #8358786 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7616c2c9ddee
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.
Description
•