Issue 7346044: Added support for non-separable blending modes
|
Created:
11 years, 2 months ago by RikC Modified:
11 years, 2 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdded 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 : #
MessagesTotal messages: 44
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?
Sign in to reply to this message.
These modes are directly supported by PDF, please add entries to blend_mode_from_xfermode() in src/pdf/SkPDFGraphicState.cpp
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
include/core/SkColorPriv.h SkComputeLuminance(r, g, b)
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Is there anything else that is needed for this patch?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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...
Sign in to reply to this message.
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.
Sign in to reply to this message.
PDF code LGTM. reed1 is on EST, so you probably won't get a final ok until tomorrow.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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... >
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
> > > > 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.
Sign in to reply to this message.
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?)
Sign in to reply to this message.
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).
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
On 2013/02/26 13:56:06, reed1 wrote: > lgtm Great! Does that mean it will be submitted?
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
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'
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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/
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
