|
|
|
Created:
12 years, 11 months ago by RikC Modified:
12 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Added support for non-separable blending modes
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total messages: 44
|
reed1
https://codereview.appspot.com/7346044/diff/1/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.appspot.com/7346044/diff/1/include/core/SkXfermode.h#newcode122 include/core/SkXfermode.h:122: I don't see these documented on http://www.w3.org/TR/2009/WD-SVGCompositing-20090430. Can you ...
|
12 years, 10 months ago (2013年02月20日 17:56:02 UTC) #1 | ||||||||||||||||||||||||||||||||||||||
https://codereview.appspot.com/7346044/diff/1/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.appspot.com/7346044/diff/1/include/core/SkXfermode.h#newco... include/core/SkXfermode.h:122: I don't see these documented on http://www.w3.org/TR/2009/WD-SVGCompositing-20090430. Can you add comments to point to the spec? https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode434 src/core/SkXfermode.cpp:434: // the non-separable blend modes rely on the following formulas: const int parameter is a construct we never use in skia. int parameter is sufficient. Must we use exactly this conversion of RGB -> Luminance? There are other approximations that don't involve an int divide. A more descriptive name? rgb_to_luminance? https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode446 src/core/SkXfermode.cpp:446: Skia uses int* for var arguments, and only uses & when the parameter is const. e.g. const Foo& 's' is int here, but a float in the caller...? I don't understand how "saturation" fits into this helper function. Is this implementing some form of lerp?
These modes are directly supported by PDF, please add entries to blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp
On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > These modes are directly supported by PDF, please add entries to > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp Will do
https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode434 src/core/SkXfermode.cpp:434: // the non-separable blend modes rely on the following formulas: On 2013年02月20日 17:56:02, reed1 wrote: > const int parameter is a construct we never use in skia. int parameter is > sufficient. > > Must we use exactly this conversion of RGB -> Luminance? There are other > approximations that don't involve an int divide. Do you have a pointer to those? > > A more descriptive name? rgb_to_luminance? I prefer to keep the name since it matches the spec (and other implementations) https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode446 src/core/SkXfermode.cpp:446: On 2013年02月20日 17:56:02, reed1 wrote: > Skia uses int* for var arguments, and only uses & when the parameter is const. > e.g. const Foo& > > 's' is int here, but a float in the caller...? > I will fix this. > I don't understand how "saturation" fits into this helper function. Is this > implementing some form of lerp? No, it's not like lerp. I'm unsure how the formula is derived; this is how it's defined in the pdf reference manual
On 2013年02月20日 18:20:42, RikC wrote: > https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp > File src/core/SkXfermode.cpp (right): > > https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode434 > src/core/SkXfermode.cpp:434: // the non-separable blend modes rely on the > following formulas: > On 2013年02月20日 17:56:02, reed1 wrote: > > const int parameter is a construct we never use in skia. int parameter is > > sufficient. > > > > Must we use exactly this conversion of RGB -> Luminance? There are other > > approximations that don't involve an int divide. > > Do you have a pointer to those? > > > > > A more descriptive name? rgb_to_luminance? > > I prefer to keep the name since it matches the spec (and other implementations) Then perhaps we can group these, perhaps via a common prefix/suffix so they are clearly all part of a set of PDF-specific helper functions. Perhaps we move all of this new support code into a separate .cpp (e.g. SkXfermodes_something.cpp), since the current file is getter awfully big anyway. > > https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode446 > src/core/SkXfermode.cpp:446: > On 2013年02月20日 17:56:02, reed1 wrote: > > Skia uses int* for var arguments, and only uses & when the parameter is const. > > e.g. const Foo& > > > > 's' is int here, but a float in the caller...? > > > > I will fix this. > > > I don't understand how "saturation" fits into this helper function. Is this > > implementing some form of lerp? > No, it's not like lerp. > I'm unsure how the formula is derived; this is how it's defined in the pdf > reference manual OK. Lets add a comment stating that, so the next reader/maintainer knows where to look to understand that function.
include/core/SkColorPriv.h SkComputeLuminance(r, g, b)
Added missing blend modes. Also, fixed the multiply blend mode. It was incorrectly using modulate https://codereview.appspot.com/7346044/diff/1/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.appspot.com/7346044/diff/1/include/core/SkXfermode.h#newco... include/core/SkXfermode.h:122: On 2013年02月20日 17:56:02, reed1 wrote: > I don't see these documented on > http://www.w3.org/TR/2009/WD-SVGCompositing-20090430. Can you add comments to > point to the spec? Done. https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode434 src/core/SkXfermode.cpp:434: // the non-separable blend modes rely on the following formulas: On 2013年02月20日 17:56:02, reed1 wrote: > const int parameter is a construct we never use in skia. int parameter is > sufficient. > > Must we use exactly this conversion of RGB -> Luminance? There are other > approximations that don't involve an int divide. > > A more descriptive name? rgb_to_luminance? Done. https://codereview.appspot.com/7346044/diff/1/src/core/SkXfermode.cpp#newcode446 src/core/SkXfermode.cpp:446: On 2013年02月20日 18:20:42, RikC wrote: > On 2013年02月20日 17:56:02, reed1 wrote: > > Skia uses int* for var arguments, and only uses & when the parameter is const. > > e.g. const Foo& > > > > 's' is int here, but a float in the caller...? > > > > I will fix this. > > > I don't understand how "saturation" fits into this helper function. Is this > > implementing some form of lerp? > No, it's not like lerp. > I'm unsure how the formula is derived; this is how it's defined in the pdf > reference manual > Done.
On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > These modes are directly supported by PDF, please add entries to > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp Is there a location where this is used to render PDFs?
On 2013年02月20日 19:58:24, RikC wrote: > On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > > These modes are directly supported by PDF, please add entries to > > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp > > Is there a location where this is used to render PDFs? I'm sorry, I'm not sure what you mean. The gm you modified should generate pdfs unless you pass the --nopdf option. https://codereview.appspot.com/7346044/diff/8001/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.appspot.com/7346044/diff/8001/src/pdf/SkPDFGraphicState.cp... src/pdf/SkPDFGraphicState.cpp:49: case SkXfermode::kModulate_Mode: If I understand correctly, Multiply and Modulate are pretty similar. It might be better to do Multiply when Modulate is requested instead of doing nothing.
On 2013年02月20日 20:18:53, Steve VanDeBogart wrote: > On 2013年02月20日 19:58:24, RikC wrote: > > On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > > > These modes are directly supported by PDF, please add entries to > > > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp > > > > Is there a location where this is used to render PDFs? > > I'm sorry, I'm not sure what you mean. The gm you modified should generate pdfs > unless you pass the --nopdf option. I was talking about the code that renders PDF files in Chrome. Does it use these routines from Skia? > > https://codereview.appspot.com/7346044/diff/8001/src/pdf/SkPDFGraphicState.cpp > File src/pdf/SkPDFGraphicState.cpp (right): > > https://codereview.appspot.com/7346044/diff/8001/src/pdf/SkPDFGraphicState.cp... > src/pdf/SkPDFGraphicState.cpp:49: case SkXfermode::kModulate_Mode: > If I understand correctly, Multiply and Modulate are pretty similar. It might > be better to do Multiply when Modulate is requested instead of doing nothing. I agree. Will do.
https://codereview.appspot.com/7346044/diff/8001/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.appspot.com/7346044/diff/8001/src/pdf/SkPDFGraphicState.cp... src/pdf/SkPDFGraphicState.cpp:49: case SkXfermode::kModulate_Mode: On 2013年02月20日 20:18:53, Steve VanDeBogart wrote: > If I understand correctly, Multiply and Modulate are pretty similar. It might > be better to do Multiply when Modulate is requested instead of doing nothing. Done.
On 2013年02月20日 20:33:33, RikC wrote: > On 2013年02月20日 20:18:53, Steve VanDeBogart wrote: > > On 2013年02月20日 19:58:24, RikC wrote: > > > On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > > > > These modes are directly supported by PDF, please add entries to > > > > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp > > > > > > Is there a location where this is used to render PDFs? > > > > I'm sorry, I'm not sure what you mean. The gm you modified should generate > pdfs > > unless you pass the --nopdf option. > > I was talking about the code that renders PDF files in Chrome. Does it use these > routines from Skia? Chrome print preview generates PDFs and that is done with Skia, yes. I'm not sure if you can trigger any of these blend modes in a printing context from Chrome, but if you can you should be able to see the result there.
On 2013年02月20日 21:06:17, Steve VanDeBogart wrote: > On 2013年02月20日 20:33:33, RikC wrote: > > On 2013年02月20日 20:18:53, Steve VanDeBogart wrote: > > > On 2013年02月20日 19:58:24, RikC wrote: > > > > On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > > > > > These modes are directly supported by PDF, please add entries to > > > > > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp > > > > > > > > Is there a location where this is used to render PDFs? > > > > > > I'm sorry, I'm not sure what you mean. The gm you modified should generate > > pdfs > > > unless you pass the --nopdf option. > > > > I was talking about the code that renders PDF files in Chrome. Does it use > these > > routines from Skia? > > Chrome print preview generates PDFs and that is done with Skia, yes. I'm not > sure if you can trigger any of these blend modes in a printing context from > Chrome, but if you can you should be able to see the result there. Chrome opens PDF files right in its window without the use of Acrobat. Is the code that renders PDFs using Skia. If so, it seems that it should be extended for these blend modes.
On 2013年02月20日 21:24:53, RikC wrote: > On 2013年02月20日 21:06:17, Steve VanDeBogart wrote: > > On 2013年02月20日 20:33:33, RikC wrote: > > > On 2013年02月20日 20:18:53, Steve VanDeBogart wrote: > > > > On 2013年02月20日 19:58:24, RikC wrote: > > > > > On 2013年02月20日 18:11:37, Steve VanDeBogart wrote: > > > > > > These modes are directly supported by PDF, please add entries to > > > > > > blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp > > > > > > > > > > Is there a location where this is used to render PDFs? > > > > > > > > I'm sorry, I'm not sure what you mean. The gm you modified should > generate > > > pdfs > > > > unless you pass the --nopdf option. > > > > > > I was talking about the code that renders PDF files in Chrome. Does it use > > these > > > routines from Skia? > > > > Chrome print preview generates PDFs and that is done with Skia, yes. I'm not > > sure if you can trigger any of these blend modes in a printing context from > > Chrome, but if you can you should be able to see the result there. > > Chrome opens PDF files right in its window without the use of Acrobat. Is the > code that renders PDFs using Skia. If so, it seems that it should be extended > for these blend modes. That code doesn't use Skia. The Skia PDF code only generates PDF, it doesn't parse and render PDFs. If the blend modes don't show up in print preview, but do if you save the PDF and view it in Acrobat, let me know and we can file a Chrome bug for that.
Is there anything else that is needed for this patch?
PDF code LGTM with nit https://codereview.appspot.com/7346044/diff/16001/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.appspot.com/7346044/diff/16001/src/pdf/SkPDFGraphicState.c... src/pdf/SkPDFGraphicState.cpp:17: case SkXfermode::kModulate_Mode: nit: add a comment saying that one of these is wrong, but close enough.
On 2013年02月22日 22:54:56, Steve VanDeBogart wrote: > PDF code LGTM with nit > > https://codereview.appspot.com/7346044/diff/16001/src/pdf/SkPDFGraphicState.cpp > File src/pdf/SkPDFGraphicState.cpp (right): > > https://codereview.appspot.com/7346044/diff/16001/src/pdf/SkPDFGraphicState.c... > src/pdf/SkPDFGraphicState.cpp:17: case SkXfermode::kModulate_Mode: > nit: add a comment saying that one of these is wrong, but close enough. Done.
https://codereview.appspot.com/7346044/diff/19001/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.appspot.com/7346044/diff/19001/src/pdf/SkPDFGraphicState.c... src/pdf/SkPDFGraphicState.cpp:17: case SkXfermode::kModulate_Mode: // kModulate is not really like multipy but similar most of the time. Sorry, 100 col limit...
https://codereview.appspot.com/7346044/diff/19001/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.appspot.com/7346044/diff/19001/src/pdf/SkPDFGraphicState.c... src/pdf/SkPDFGraphicState.cpp:17: case SkXfermode::kModulate_Mode: // kModulate is not really like multipy but similar most of the time. On 2013年02月22日 23:10:25, Steve VanDeBogart wrote: > Sorry, 100 col limit... Done.
PDF code LGTM. reed1 is on EST, so you probably won't get a final ok until tomorrow.
https://codereview.appspot.com/7346044/diff/22001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.appspot.com/7346044/diff/22001/src/core/SkXfermode.cpp#new... src/core/SkXfermode.cpp:435: // (See https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#blendingnonsep...) Why not call SkComputeLuminance?
Do we already have benchs that exercise each of the xfermodes? These new ones look complex enough that we may want to have such a bench(s) so we can experiment with alternative impls.
https://codereview.appspot.com/7346044/diff/22001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.appspot.com/7346044/diff/22001/src/core/SkXfermode.cpp#new... src/core/SkXfermode.cpp:435: // (See https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#blendingnonsep...) On 2013年02月25日 13:57:43, reed1 wrote: > Why not call SkComputeLuminance? That routine uses different values to multiply the components.
I'm unsure about that. is that a benchmark suite that skia runs? gm/xfermodes.cpp <https://codereview.appspot.com/7346044/patch/22001/16002> exercises each blend mode. Rik On Mon, Feb 25, 2013 at 5:58 AM, <reed@google.com> wrote: > Do we already have benchs that exercise each of the xfermodes? These new > ones look complex enough that we may want to have such a bench(s) so we > can experiment with alternative impls. > > https://codereview.appspot.**com/7346044/<https://codereview.appspot.com/7346... >
The coefficients we use for Luminance are derived from http://www.itu.int/rec/R-REC-BT.709/ Our understanding is that these are also referenced by CSS itself (adding Ben for more details). 1. Is there a correct set of coefficients we should use throughout Skia? 2. If so, lets all use SkComputeLuminance, and (as needed) correct its coefficients 3. If not, please add a comment above your Lum() local function documenting that it differs from SkComputeLuminance, and why. I guess we don't have an existing micro-bench for individual xfermodes. I will work to add one, so this CL need not be held up for that.
On 2013年02月25日 17:43:47, reed1 wrote: > The coefficients we use for Luminance are derived from > http://www.itu.int/rec/R-REC-BT.709/ > > Our understanding is that these are also referenced by CSS itself (adding Ben > for more details). > > 1. Is there a correct set of coefficients we should use throughout Skia? The one that the blend modes are using, are derived from NTSC (see PDF reference manual 1.7, section 6.2.1) > 2. If so, lets all use SkComputeLuminance, and (as needed) correct its > coefficients > 3. If not, please add a comment above your Lum() local function documenting that > it differs from SkComputeLuminance, and why. I'm unsure which one is 'better'. Will there ever be a case where css luminance is expected to be the same as blending? (Where is that used?) If yes, they should be the same. > > I guess we don't have an existing micro-bench for individual xfermodes. I will > work to add one, so this CL need not be held up for that.
On 2013年02月25日 18:33:00, RikC wrote: > On 2013年02月25日 17:43:47, reed1 wrote: > > The coefficients we use for Luminance are derived from > > http://www.itu.int/rec/R-REC-BT.709/ > > > > Our understanding is that these are also referenced by CSS itself (adding Ben > > for more details). > > > > 1. Is there a correct set of coefficients we should use throughout Skia? > > The one that the blend modes are using, are derived from NTSC (see PDF reference > manual 1.7, section 6.2.1) Drive-by comment: IIRC, the YCbCr computation is different between NTSC SD (ITU 601) and NTSC HD (ITU 709). Strange but true. http://www.glennchan.info/articles/technical/hd-versus-sd-color-space/hd-vers... > > 2. If so, lets all use SkComputeLuminance, and (as needed) correct its > > coefficients > > 3. If not, please add a comment above your Lum() local function documenting > that > > it differs from SkComputeLuminance, and why. > > I'm unsure which one is 'better'. Will there ever be a case where css luminance > is expected to be the same as blending? (Where is that used?) > If yes, they should be the same. > > > > > I guess we don't have an existing micro-bench for individual xfermodes. I will > > work to add one, so this CL need not be held up for that.
On 2013年02月25日 18:39:48, Stephen White wrote: > On 2013年02月25日 18:33:00, RikC wrote: > > On 2013年02月25日 17:43:47, reed1 wrote: > > > The coefficients we use for Luminance are derived from > > > http://www.itu.int/rec/R-REC-BT.709/ > > > > > > Our understanding is that these are also referenced by CSS itself (adding > Ben > > > for more details). > > > > > > 1. Is there a correct set of coefficients we should use throughout Skia? > > > > The one that the blend modes are using, are derived from NTSC (see PDF > reference > > manual 1.7, section 6.2.1) > > Drive-by comment: IIRC, the YCbCr computation is different between NTSC SD (ITU > 601) and NTSC HD (ITU 709). Strange but true. yes, it seems that we're running into this. Skia is 709 and Blending is 601. Interesting! > > http://www.glennchan.info/articles/technical/hd-versus-sd-color-space/hd-vers... > > > > 2. If so, lets all use SkComputeLuminance, and (as needed) correct its > > > coefficients > > > 3. If not, please add a comment above your Lum() local function documenting > > that > > > it differs from SkComputeLuminance, and why. > > > > I'm unsure which one is 'better'. Will there ever be a case where css > luminance > > is expected to be the same as blending? (Where is that used?) > > If yes, they should be the same. > > > > > > > > I guess we don't have an existing micro-bench for individual xfermodes. I > will > > > work to add one, so this CL need not be held up for that.
On 2013年02月25日 18:49:13, RikC wrote: > On 2013年02月25日 18:39:48, Stephen White wrote: > > On 2013年02月25日 18:33:00, RikC wrote: > > > On 2013年02月25日 17:43:47, reed1 wrote: > > > > The coefficients we use for Luminance are derived from > > > > http://www.itu.int/rec/R-REC-BT.709/ > > > > > > > > Our understanding is that these are also referenced by CSS itself (adding > > Ben > > > > for more details). > > > > > > > > 1. Is there a correct set of coefficients we should use throughout Skia? > > > > > > The one that the blend modes are using, are derived from NTSC (see PDF > > reference > > > manual 1.7, section 6.2.1) > > > > Drive-by comment: IIRC, the YCbCr computation is different between NTSC SD > (ITU > > 601) and NTSC HD (ITU 709). Strange but true. > > yes, it seems that we're running into this. Skia is 709 and Blending is 601. > Interesting! > Yes, the reason to go with 709 in Skia is primarily that CSS says all colors are sRGB (unless they aren't). The sRGB specification (from color.org, summary of the sRGB portion of iec 61966-2-1) references 709. > > > > > http://www.glennchan.info/articles/technical/hd-versus-sd-color-space/hd-vers... > > > > > > 2. If so, lets all use SkComputeLuminance, and (as needed) correct its > > > > coefficients > > > > 3. If not, please add a comment above your Lum() local function > documenting > > > that > > > > it differs from SkComputeLuminance, and why. > > > > > > I'm unsure which one is 'better'. Will there ever be a case where css > > luminance > > > is expected to be the same as blending? (Where is that used?) > > > If yes, they should be the same. > > > > > > > > > > > I guess we don't have an existing micro-bench for individual xfermodes. I > > will > > > > work to add one, so this CL need not be held up for that.
On 2013年02月25日 19:10:54, bungeman wrote: > On 2013年02月25日 18:49:13, RikC wrote: > > On 2013年02月25日 18:39:48, Stephen White wrote: > > > On 2013年02月25日 18:33:00, RikC wrote: > > > > On 2013年02月25日 17:43:47, reed1 wrote: > > > > > The coefficients we use for Luminance are derived from > > > > > http://www.itu.int/rec/R-REC-BT.709/ > > > > > > > > > > Our understanding is that these are also referenced by CSS itself > (adding > > > Ben > > > > > for more details). > > > > > > > > > > 1. Is there a correct set of coefficients we should use throughout Skia? > > > > > > > > The one that the blend modes are using, are derived from NTSC (see PDF > > > reference > > > > manual 1.7, section 6.2.1) > > > > > > Drive-by comment: IIRC, the YCbCr computation is different between NTSC SD > > (ITU > > > 601) and NTSC HD (ITU 709). Strange but true. > > > > yes, it seems that we're running into this. Skia is 709 and Blending is 601. > > Interesting! > > > > Yes, the reason to go with 709 in Skia is primarily that CSS says all colors are > sRGB (unless they aren't). The sRGB specification (from http://color.org, summary of > the sRGB portion of iec 61966-2-1) references 709. > Which one? there's more than 1 sRGB standard :-) It seems fine that you're moving to the new one, as long as they're not expected to match. Where are you using luminance in skia/chrome? > > > > > > > > > http://www.glennchan.info/articles/technical/hd-versus-sd-color-space/hd-vers... > > > > > > > > 2. If so, lets all use SkComputeLuminance, and (as needed) correct its > > > > > coefficients > > > > > 3. If not, please add a comment above your Lum() local function > > documenting > > > > that > > > > > it differs from SkComputeLuminance, and why. > > > > > > > > I'm unsure which one is 'better'. Will there ever be a case where css > > > luminance > > > > is expected to be the same as blending? (Where is that used?) > > > > If yes, they should be the same. > > > > > > > > > > > > > > I guess we don't have an existing micro-bench for individual xfermodes. > I > > > will > > > > > work to add one, so this CL need not be held up for that.
> > > > Yes, the reason to go with 709 in Skia is primarily that CSS says all colors > are > > sRGB (unless they aren't). The sRGB specification (from http://color.org, > summary of > > the sRGB portion of iec 61966-2-1) references 709. > > > > Which one? there's more than 1 sRGB standard :-) > It seems fine that you're moving to the new one, as long as they're not expected > to match. Where are you using luminance in skia/chrome? > This one http://webstore.iec.ch/webstore/webstore.nsf/Artnum_PK/25408 I think the only thing it's really used for is back-forming regular anti-aliased glyphs from lcd anti-aliased glyphs, and figuring out which gamma hack to use. So not much at the moment. Currently only the text blits take any color space information into account, the rest of the blits are done linear.
On 2013年02月25日 19:44:48, bungeman wrote: > > > > > > Yes, the reason to go with 709 in Skia is primarily that CSS says all colors > > are > > > sRGB (unless they aren't). The sRGB specification (from http://color.org, > > summary of > > > the sRGB portion of iec 61966-2-1) references 709. > > > > > > > Which one? there's more than 1 sRGB standard :-) > > It seems fine that you're moving to the new one, as long as they're not > expected > > to match. Where are you using luminance in skia/chrome? > > > > This one > http://webstore.iec.ch/webstore/webstore.nsf/Artnum_PK/25408 > > I think the only thing it's really used for is back-forming regular anti-aliased > glyphs from lcd anti-aliased glyphs, and figuring out which gamma hack to use. > So not much at the moment. Currently only the text blits take any color space > information into account, the rest of the blits are done linear. So, should we update that to use the old values? It seems that keeping them separate would be less invasive since this will change the rendering a lot. (PS Are you adjusting for gamma as well for glyph AA?)
It is unfortunate that this spec has chosen the older values. If that cannot be corrected/changed (?) then I agree we should just agree to have a private impl for luminance w/ these new xfermodes. Please add a comment in Lum() acknowledging that SkComputeLuminance exists, but that your spec requires different coefficients (so the next reader does not ask the same question).
On 2013年02月25日 21:05:23, reed1 wrote: > It is unfortunate that this spec has chosen the older values. If that cannot be > corrected/changed (?) The blending spec was written 15 years or so ago and integrated into a bunch of applications, frameworks and operating systems so unfortunately it can't be changed. (This is actually not a big deal since the formula is not really trying to work with actual luminosity) > then I agree we should just agree to have a private impl > for luminance w/ these new xfermodes. Please add a comment in Lum() > acknowledging that SkComputeLuminance exists, but that your spec requires > different coefficients (so the next reader does not ask the same question). Will do.
On 2013年02月25日 21:05:23, reed1 wrote: > It is unfortunate that this spec has chosen the older values. If that cannot be > corrected/changed (?) then I agree we should just agree to have a private impl > for luminance w/ these new xfermodes. Please add a comment in Lum() > acknowledging that SkComputeLuminance exists, but that your spec requires > different coefficients (so the next reader does not ask the same question). Done
lgtm
On 2013年02月26日 13:56:06, reed1 wrote: > lgtm Great! Does that mean it will be submitted?
On 2013年02月26日 13:56:06, reed1 wrote: > lgtm Can it be submitted to the depot? Has any work been done on the GPU implementation?
On 2013年02月27日 16:31:49, RikC wrote: > On 2013年02月26日 13:56:06, reed1 wrote: > > lgtm > > Can it be submitted to the depot? > Has any work been done on the GPU implementation? ping
Some warnings that need to be fixed (we treat these as errors on some of our bots) cc1plus: warnings being treated as errors /skia2/trunk/gyp/../src/core/SkXfermode.cpp: In function 'int Sat(int, int, int)': /skia2/trunk/gyp/../src/core/SkXfermode.cpp:448: warning: converting to 'int' from 'SkScalar' /skia2/trunk/gyp/../src/core/SkXfermode.cpp: In function 'void SetSat(int*, int*, int*, float)': /skia2/trunk/gyp/../src/core/SkXfermode.cpp:466: warning: passing 'float' for argument 4 to 'void setSaturationComponents(int*, int*, int*, int)' /skia2/trunk/gyp/../src/core/SkXfermode.cpp:468: warning: passing 'float' for argument 4 to 'void setSaturationComponents(int*, int*, int*, int)' /skia2/trunk/gyp/../src/core/SkXfermode.cpp:470: warning: passing 'float' for argument 4 to 'void setSaturationComponents(int*, int*, int*, int)' /skia2/trunk/gyp/../src/core/SkXfermode.cpp:473: warning: passing 'float' for argument 4 to 'void setSaturationComponents(int*, int*, int*, int)' /skia2/trunk/gyp/../src/core/SkXfermode.cpp:475: warning: passing 'float' for argument 4 to 'void setSaturationComponents(int*, int*, int*, int)' /skia2/trunk/gyp/../src/core/SkXfermode.cpp:477: warning: passing 'float' for argument 4 to 'void setSaturationComponents(int*, int*, int*, int)' /skia2/trunk/gyp/../src/core/SkXfermode.cpp: In function 'void clipColor(int*, int*, int*)': /skia2/trunk/gyp/../src/core/SkXfermode.cpp:483: warning: converting to 'int' from 'SkScalar' /skia2/trunk/gyp/../src/core/SkXfermode.cpp:484: warning: converting to 'int' from 'SkScalar'
Perhaps you should start a new CL... if you do, it will be hosted by chromium which has the nice side-effect of offering try-bots, so you can see these warnings/errors yourself.
On 2013年03月04日 17:17:12, reed1 wrote: > Perhaps you should start a new CL... if you do, it will be hosted by chromium > which has the nice side-effect of offering try-bots, so you can see these > warnings/errors yourself. Is that a CL on skia or chromium? How is it hosted by chromium?
the chromium part of the URL is a side-effect of us trying to share code. the CL will *not* be part of chrome per-se. If you sync, and re-run 'gcl change foo', it should create a new CL with the new backend.
On 2013年03月04日 17:27:04, reed1 wrote: > the chromium part of the URL is a side-effect of us trying to share code. the CL > will *not* be part of chrome per-se. > > If you sync, and re-run 'gcl change foo', it should create a new CL with the new > backend. created new cl https://codereview.chromium.org/12393049/