Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288323004:
Patches on top of Radu's latest. (Closed)
DescriptionPatches on top of Radu's latest.
patch from issue 1273033005 at patchset 120001 (http://crrev.com/1273033005#ps120001)
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/2d141ba2df8f7506848aa9369f502944e837cd09
Committed: https://skia.googlesource.com/skia/+/3b44dcf078d7365c11b5a20916a9b18a75498b56
Patch Set 1 #Patch Set 2 : fmt #Patch Set 3 : char #Patch Set 4 : fix16 #Patch Set 5 : rebase #Patch Set 6 : inline #Patch Set 7 : g0b0 #Patch Set 8 : const #
Total comments: 8
Patch Set 9 : noel #Messages
Total messages: 51 (15 generated)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288323004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288323004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288323004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288323004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288323004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288323004/140001
mtklein@chromium.org changed reviewers: + noel@chromium.org, radu.velea@intel.com
Updated with a few things Radu mentioned when we chatted briefly this morning. PTAL. I don't mind what we land or under whose name, so we can patch this (or some of this) back to the original if you like.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/08/18 14:09:51, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm
The CQ bit was checked by mtklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288323004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288323004/140001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-08-18 22:38 UTC
mtklein@google.com changed reviewers: + mtklein@google.com
lgtm
The CQ bit was unchecked by mtklein@chromium.org
The CQ bit was checked by mtklein@chromium.org
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/2d141ba2df8f7506848aa9369f502944e837cd09
Message was sent while issue was closed.
On 2015/08/18 16:43:31, commit-bot: I haz the power wrote: > Committed patchset #8 (id:140001) as > https://skia.googlesource.com/skia/+/2d141ba2df8f7506848aa9369f502944e837cd09 Looking pretty good on our perf bots: https://perf.skia.org/#3927
Message was sent while issue was closed.
On 2015/08/18 18:36:50, mtklein_C wrote: > On 2015/08/18 16:43:31, commit-bot: I haz the power wrote: > > Committed patchset #8 (id:140001) as > > https://skia.googlesource.com/skia/+/2d141ba2df8f7506848aa9369f502944e837cd09 > > Looking pretty good on our perf bots: https://perf.skia.org/#3927 Yes the results look good there. Patch is already submitted, but some comments form a different time-zone ...
Message was sent while issue was closed.
On 2015/08/19 00:40:20, noel gordon wrote: > On 2015/08/18 18:36:50, mtklein_C wrote: > > On 2015/08/18 16:43:31, commit-bot: I haz the power wrote: > > > Committed patchset #8 (id:140001) as > > > > https://skia.googlesource.com/skia/+/2d141ba2df8f7506848aa9369f502944e837cd09 > > > > Looking pretty good on our perf bots: https://perf.skia.org/#3927 > > Yes the results look good there. Patch is already submitted, but some comments > form a different time-zone ... Sure thing... I'm happy to follow up or review such a follow-up. Were there some comments queued up here that didn't get published?
Message was sent while issue was closed.
I measure this patch again on my mac air OSX 10.10. Before the patch we were around 320ms for 33 cube in color cube bench. With the current patch, 140ms for a 33 cube. We lost a little (~10%) when we switched to using FromBGRx(), but that's unavoidable if we want correctness in SampleApp. I will measure perf on win32 later today. One thing I noticed with SampleApp --slide GM:colorcube is that there were more off-by-one pixel changes (when comparing with the GPU backend results) than with the original software implementation. Not sure why that's happening. Other comments inline ... https://codereview.chromium.org/1288323004/diff/140001/src/core/SkPMFloat.h File src/core/SkPMFloat.h (right): https://codereview.chromium.org/1288323004/diff/140001/src/core/SkPMFloat.h#n... src/core/SkPMFloat.h:29: static SkPMFloat FromBGRx(SkColor c); // Ignores c's alpha, instead forcing it to 1. The CL lacks a decent description, imho. We could have mentioned the need for this new function for example, and added a few comments about perf benefits. https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... File src/effects/SkColorCubeFilter.cpp (left): https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... src/effects/SkColorCubeFilter.cpp:160: } This was a reference implementation, and it was a tricky algorithm. Would it make sense to retain this code in #ifdef or something for reference? https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... File src/effects/SkColorCubeFilter.cpp (right): https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... src/effects/SkColorCubeFilter.cpp:134: (SkColor*)fCubeData->data()); (const SkColor*)fCubeData->data() ? https://codereview.chromium.org/1288323004/diff/140001/src/opts/SkColorCubeFi... File src/opts/SkColorCubeFilter_opts.h (right): https://codereview.chromium.org/1288323004/diff/140001/src/opts/SkColorCubeFi... src/opts/SkColorCubeFilter_opts.h:73: color = color * Sk4f(((float)a) / 255); I think clang will generate a DIV instruction in the ASM here? Perhaps this should be color = color * Sk4f(a * (1.0f / 255.0f)); to make the compiler generate a MUL instruction instead?
Message was sent while issue was closed.
> We lost a little (~10%) when we switched to using FromBGRx(), but that's > unavoidable if we want correctness in SampleApp. I will measure perf on win32 > later today. I think this might be avoidable if we change the API of SkColorCube to require 0xFF alpha rather than ignore it and treat it like it's 0xFF. We could probably do even better if we specified the color cube in terms of opaque SkPMColors, so there's no swizzle required either. Maybe this can be done in the constructor? I'm not sure I understand why the cube is passed as an SkData. Is that to allow recounting? > One thing I noticed with SampleApp --slide GM:colorcube is that there were more > off-by-one pixel changes (when comparing with the GPU backend results) than with > the original software implementation. Not sure why that's happening. Hmm. That's pretty weird. We didn't generate any new images for the colorcube GM when this landed. I might be reading this (https://gold.skia.org/cluster?query=source_type%3Dgm%26name%3Dcolorcube&head=...) wrong but it doesn't look like any images have changed or any configurations swapped from one good image to another. There's one good image (46322b8d0a9faab401110e42f0c2fb79) that seems to cover all the software 8888, one for all software 565 (25d5894856d49f0033c09736c1ce7cd7), then a bunch for various GPUs. It's really an exemplar of consistency as far as GMs go.
Message was sent while issue was closed.
New PS 9 up! https://codereview.chromium.org/1288323004/diff/140001/src/core/SkPMFloat.h File src/core/SkPMFloat.h (right): https://codereview.chromium.org/1288323004/diff/140001/src/core/SkPMFloat.h#n... src/core/SkPMFloat.h:29: static SkPMFloat FromBGRx(SkColor c); // Ignores c's alpha, instead forcing it to 1. On 2015/08/19 01:02:59, noel gordon wrote: > The CL lacks a decent description, imho. We could have mentioned the need for > this new function for example, and added a few comments about perf benefits. Have we not got all that context over at 1273033005? https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... File src/effects/SkColorCubeFilter.cpp (left): https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... src/effects/SkColorCubeFilter.cpp:160: } On 2015/08/19 01:02:59, noel gordon wrote: > This was a reference implementation, and it was a tricky algorithm. Would it > make sense to retain this code in #ifdef or something for reference? I don't think so, but I don't mind adding it if you like. In my experience code that's not actively compiled and tested swiftly becomes out of date. https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... File src/effects/SkColorCubeFilter.cpp (right): https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... src/effects/SkColorCubeFilter.cpp:134: (SkColor*)fCubeData->data()); On 2015/08/19 01:02:59, noel gordon wrote: > (const SkColor*)fCubeData->data() ? Done. https://codereview.chromium.org/1288323004/diff/140001/src/opts/SkColorCubeFi... File src/opts/SkColorCubeFilter_opts.h (right): https://codereview.chromium.org/1288323004/diff/140001/src/opts/SkColorCubeFi... src/opts/SkColorCubeFilter_opts.h:73: color = color * Sk4f(((float)a) / 255); On 2015/08/19 01:02:59, noel gordon wrote: > I think clang will generate a DIV instruction in the ASM here? Perhaps this > should be > > color = color * Sk4f(a * (1.0f / 255.0f)); > > to make the compiler generate a MUL instruction instead? Yep, looks like I overlooked this. Looks like it makes about a 2% difference if I modify the bench to use non-opaque colors.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288323004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288323004/160001
> Maybe this can be done in the constructor? I'm not sure I understand why the > cube is passed as an SkData. Is that to allow recounting? *refcounting
The CQ bit was unchecked by mtklein@google.com
On 2015/08/19 01:03:00, noel gordon wrote: > We lost a little (~10%) when we switched to using FromBGRx(), but that's > unavoidable if we want correctness in SampleApp. I will measure perf on win32 > later today. % /out/Release/qcms_test.exe -w 2880 -h 1800 -i 40 Test qcms clut transforms for 40 iterations Test image size 2880 x 1800 pixels 23.442380 (avg 0.586059) seconds qcms_transform_data_tetra_clut_rgba 5.757392 (avg 0.143935) seconds qcms_transform_data_tetra_clut_rgba_sse2 4.071701 speedup after 40 iterations So 143ms on todays chrome on win7 z620, and this skia color cube code: 151ms. That is a very good result, considering we lost 10%, and we are comparing a tetrahedral interpolator (chrome) to a tri-linear interpolator (skia color cube filter).
On 2015/08/19 01:57:53, mtklein wrote: > I'm not sure I understand why the > cube is passed as an SkData. Is that to allow recounting? > *refcounting Yes, refcounting. Cubes are 33 x 33 x 33 x RGBA, so can be large, and there are instances where we can share the cube data with multiple color cube filters active on a web page. The refcounting helps save memory. > > One thing I noticed with SampleApp --slide GM:colorcube is that there were > more > > off-by-one pixel changes (when comparing with the GPU backend results) than > with > > the original software implementation. Not sure why that's happening. > > Hmm. That's pretty weird. We didn't generate any new images for the colorcube > GM when this landed. I might be reading this > (https://gold.skia.org/cluster?query=source_type%3Dgm%26name%3Dcolorcube&head=...) > wrong but it doesn't look like any images have changed or any configurations > swapped from one good image to another. There's one good image > (46322b8d0a9faab401110e42f0c2fb79) that seems to cover all the software 8888, > one for all software 565 (25d5894856d49f0033c09736c1ce7cd7), then a bunch for > various GPUs. It's really an exemplar of consistency as far as GMs go. When I measured on win, I also looked at sampleApp before and after this change, and did not see as many off-by-ones compared to mac. Perhaps it's a function of the actual GPU hardware / drivers etc. If the GM look exemplar to you, I'm good with that, and thanks for checking.
On 2015/08/19 17:06:13, noel gordon wrote: > On 2015/08/19 01:57:53, mtklein wrote: > > > I'm not sure I understand why the > > cube is passed as an SkData. Is that to allow recounting? > > *refcounting > > Yes, refcounting. Cubes are 33 x 33 x 33 x RGBA, so can be large, and there are > instances where we can share the cube data with multiple color cube filters > active on a web page. The refcounting helps save memory. Gotcha. Can we not share the SkColorCubeFilter instead? That's refcounted too. > > > One thing I noticed with SampleApp --slide GM:colorcube is that there were > > more > > > off-by-one pixel changes (when comparing with the GPU backend results) than > > with > > > the original software implementation. Not sure why that's happening. > > > > Hmm. That's pretty weird. We didn't generate any new images for the > colorcube > > GM when this landed. I might be reading this > > > (https://gold.skia.org/cluster?query=source_type%3Dgm%26name%3Dcolorcube&head=...) > > wrong but it doesn't look like any images have changed or any configurations > > swapped from one good image to another. There's one good image > > (46322b8d0a9faab401110e42f0c2fb79) that seems to cover all the software 8888, > > one for all software 565 (25d5894856d49f0033c09736c1ce7cd7), then a bunch for > > various GPUs. It's really an exemplar of consistency as far as GMs go. > > When I measured on win, I also looked at sampleApp before and after this change, > and did not see as many off-by-ones compared to mac. Perhaps it's a function of > the actual GPU hardware / drivers etc. If the GM look exemplar to you, I'm good > with that, and thanks for checking. Ah yeah, definitely the GPU side of things can vary. I believe the software side has not changed.
On 2015/08/19 01:57:53, mtklein wrote: > > We lost a little (~10%) when we switched to using FromBGRx(), but that's > > unavoidable if we want correctness in SampleApp. I will measure perf on win32 > > later today. > > I think this might be avoidable if we change the API of SkColorCube to require > 0xFF alpha rather than ignore it and treat it like it's 0xFF. You can assume this. When we create a color cube, the alpha channel is always 0xFF. The alpha channel is not used in the cube interpolation at all, and it doesn't make sense to set to to anything but 0xFF. > We could probably do even better if we specified the color cube in terms of > opaque SkPMColors, so there's no swizzle required either. Since the alpha is always 0xFF, then you have opaque SkPMColors, except they are in SkColor order. We have to use some order, so we picked the one that made the most sense, BGRA (SkColor order) since it matched desktop chrome PM color order. Maybe knowing that alpha is always 0xFF is enough to gain back that 10%?
On 2015/08/19 01:58:22, mtklein wrote: > New PS 9 up! > > https://codereview.chromium.org/1288323004/diff/140001/src/core/SkPMFloat.h > File src/core/SkPMFloat.h (right): > > https://codereview.chromium.org/1288323004/diff/140001/src/core/SkPMFloat.h#n... > src/core/SkPMFloat.h:29: static SkPMFloat FromBGRx(SkColor c); // Ignores c's > alpha, instead forcing it to 1. > On 2015/08/19 01:02:59, noel gordon wrote: > > The CL lacks a decent description, imho. We could have mentioned the need for > > this new function for example, and added a few comments about perf benefits. > > Have we not got all that context over at 1273033005? I suppose so, I just wanted to ensure Raud's contribution was noted. > https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... > File src/effects/SkColorCubeFilter.cpp (left): > > https://codereview.chromium.org/1288323004/diff/140001/src/effects/SkColorCub... > src/effects/SkColorCubeFilter.cpp:160: } > On 2015/08/19 01:02:59, noel gordon wrote: > > This was a reference implementation, and it was a tricky algorithm. Would it > > make sense to retain this code in #ifdef or something for reference? > > I don't think so, but I don't mind adding it if you like. In my experience code > that's not actively compiled and tested swiftly becomes out of date. Perhaps we could retain and make it active? For example, would it be possible to make that code the "none" implementation? > https://codereview.chromium.org/1288323004/diff/140001/src/opts/SkColorCubeFi... > src/opts/SkColorCubeFilter_opts.h:73: color = color * Sk4f(((float)a) / 255); > On 2015/08/19 01:02:59, noel gordon wrote: > > I think clang will generate a DIV instruction in the ASM here? Perhaps this > > should be > > > > color = color * Sk4f(a * (1.0f / 255.0f)); > > > > to make the compiler generate a MUL instruction instead? > > Yep, looks like I overlooked this. Looks like it makes about a 2% difference if > I modify the bench to use non-opaque colors. Wonderful. Patch LGTM btw.
On 2015/08/19 17:15:40, noel gordon wrote: > On 2015/08/19 01:57:53, mtklein wrote: > > > We lost a little (~10%) when we switched to using FromBGRx(), but that's > > > unavoidable if we want correctness in SampleApp. I will measure perf on > win32 > > > later today. > > > > I think this might be avoidable if we change the API of SkColorCube to require > > 0xFF alpha rather than ignore it and treat it like it's 0xFF. > > You can assume this. When we create a color cube, the alpha channel is always > 0xFF. The alpha channel is not used in the cube interpolation at all, and it > doesn't make sense to set to to anything but 0xFF. Ah good. I'll update the API docs and switch from ORing in 0xFF to asserting it. I guess we can call the method FromBGR1 now. > > > We could probably do even better if we specified the color cube in terms of > > opaque SkPMColors, so there's no swizzle required either. > > Since the alpha is always 0xFF, then you have opaque SkPMColors, except they are > in SkColor order. We have to use some order, so we picked the one that made the > most sense, BGRA (SkColor order) since it matched desktop chrome PM color order. > Maybe knowing that alpha is always 0xFF is enough to gain back that 10%? Desktop Chrome actually uses two different color orders, BGRA or RGBA, depending on OS. Using SkPMColor here means we'll match SkPMColor everwhere, rather than just BGRA platforms. This is probably not a big deal for speed. It won't make any difference at all on machines with SSSE3 (it's a single _mm_shuffle_epi8 either way) but it will for those lonely old machines with only SSE2 or SSE3, and maybe for ARM devices (I think Clank and Chrome OS use different orders, so one of them suffers here, don't remember which). If we were to look at using SSE4.1, then order starts to matter again, as _mm_cvtepi8_epi32 will be the yet faster way to splat out the pixel to 32-bit lanes that only works when the orders match.
On 2015/08/19 17:09:32, mtklein wrote: > On 2015/08/19 17:06:13, noel gordon wrote: > > On 2015/08/19 01:57:53, mtklein wrote: > > > > > I'm not sure I understand why the > > > cube is passed as an SkData. Is that to allow recounting? > > > *refcounting > > > > Yes, refcounting. Cubes are 33 x 33 x 33 x RGBA, so can be large, and there are > > instances where we can share the cube data with multiple color cube filters > > active on a web page. The refcounting helps save memory. > > Gotcha. Can we not share the SkColorCubeFilter instead? That's refcounted too. Might be possible, yes. Not sure how Blink will cache filters yet. Blink might have it own class to represent a color transform, the class will have a SkColorCube member, or a SkColorCurve filter (in development), and possibly both. We'll see when we get there.
> Perhaps we could retain and make it active? For example, would it be possible > to make that code the "none" implementation? That's sort of non-desirable, as we then have to maintain two implementations, and particularly wonder about what each is doing. Sending everything through this single SkOpts path usually means there are either bugs on all platforms or bugs on no platforms. Since we don't really test on platforms other than x86, x86-64, ARMv7 and ARMv8, it's spooky to have one heavily tested implementation for those preferred platforms and another not-really-tested implementation for MIPS, ARMv6, etc.
> If we were to look at using SSE4.1, then order starts to matter again, as > _mm_cvtepi8_epi32 > will be the yet faster way to splat out the pixel to 32-bit lanes that only > works when the > orders match. Err, _mm_cvtepu8_epi32. Subtle but important difference!
The CQ bit was checked by mtklein@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, radu.velea@intel.com Link to the patchset: https://codereview.chromium.org/1288323004/#ps160001 (title: "noel")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288323004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288323004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/3b44dcf078d7365c11b5a20916a9b18a75498b56
Message was sent while issue was closed.
On 2015/08/19 17:33:20, mtklein wrote: > On 2015/08/19 17:15:40, noel gordon wrote: > > On 2015/08/19 01:57:53, mtklein wrote: > > > > We lost a little (~10%) when we switched to using FromBGRx(), but that's > > > > unavoidable if we want correctness in SampleApp. I will measure perf on > > win32 > > > > later today. > > > > > > I think this might be avoidable if we change the API of SkColorCube to > require > > > 0xFF alpha rather than ignore it and treat it like it's 0xFF. > > > > You can assume this. When we create a color cube, the alpha channel is always > > 0xFF. The alpha channel is not used in the cube interpolation at all, and it > > doesn't make sense to set to to anything but 0xFF. > > Ah good. I'll update the API docs and switch from ORing in 0xFF to asserting > it. SGTM > I guess we can call the method FromBGR1 now. Perhaps FromBRG even. > > > > > We could probably do even better if we specified the color cube in terms of > > > opaque SkPMColors, so there's no swizzle required either. > > > > Since the alpha is always 0xFF, then you have opaque SkPMColors, except they > are > > in SkColor order. We have to use some order, so we picked the one that made > the > > most sense, BGRA (SkColor order) since it matched desktop chrome PM color > order. > > Maybe knowing that alpha is always 0xFF is enough to gain back that 10%? > > Desktop Chrome actually uses two different color orders, BGRA or RGBA, depending > on OS. > Using SkPMColor here means we'll match SkPMColor everwhere, rather than just > BGRA platforms. Linux Mac Win are BGRA order according to Blink image decoders. Android Chrome is different, RGBA. Color correction is targeted at the desktop machines, no call for it on mobile devices (uses too battery, screens are not good enough to warrant color correction). Some other non-mobile Android applications need color correction though. > This is probably not a big deal for speed. It won't make any difference at all > on machines > with SSSE3 (it's a single _mm_shuffle_epi8 either way) but it will for those > lonely old > machines with only SSE2 or SSE3, and maybe for ARM devices (I think Clank and > Chrome OS use > different orders, so one of them suffers here, don't remember which). ARM I think. And agree, we pixel pushers have to swizzle sometimes (for Android).
Message was sent while issue was closed.
On 2015/08/19 17:36:55, mtklein wrote: > > Perhaps we could retain and make it active? For example, would it be possible > > to make that code the "none" implementation? > > That's sort of non-desirable, as we then have to maintain two implementations, > and particularly wonder about what each is doing. Sending everything through > this single SkOpts path usually means there are either bugs on all platforms or > bugs on no platforms. Since we don't really test on platforms other than x86, > x86-64, ARMv7 and ARMv8, it's spooky to have one heavily tested implementation > for those preferred platforms and another not-really-tested implementation for > MIPS, ARMv6, etc. Ok, I suppose we can also just git blame and find the old code to work out what it is meant to be doing.
Message was sent while issue was closed.
> Perhaps FromBRG even. Ahem, *FromBGR() even.
Message was sent while issue was closed.
On 2015/08/19 17:49:25, noel gordon wrote: > On 2015/08/19 17:33:20, mtklein wrote: > > On 2015/08/19 17:15:40, noel gordon wrote: > > > On 2015/08/19 01:57:53, mtklein wrote: > > > > > We lost a little (~10%) when we switched to using FromBGRx(), but that's > > > > > unavoidable if we want correctness in SampleApp. I will measure perf on > > > win32 > > > > > later today. > > > > > > > > I think this might be avoidable if we change the API of SkColorCube to > > require > > > > 0xFF alpha rather than ignore it and treat it like it's 0xFF. > > > > > > You can assume this. When we create a color cube, the alpha channel is > always > > > 0xFF. The alpha channel is not used in the cube interpolation at all, and > it > > > doesn't make sense to set to to anything but 0xFF. > > > > Ah good. I'll update the API docs and switch from ORing in 0xFF to asserting > > it. > > SGTM > > > I guess we can call the method FromBGR1 now. > > Perhaps FromBRG even. > > > > > > > > We could probably do even better if we specified the color cube in terms > of > > > > opaque SkPMColors, so there's no swizzle required either. > > > > > > Since the alpha is always 0xFF, then you have opaque SkPMColors, except they > > are > > > in SkColor order. We have to use some order, so we picked the one that made > > the > > > most sense, BGRA (SkColor order) since it matched desktop chrome PM color > > order. > > > Maybe knowing that alpha is always 0xFF is enough to gain back that 10%? > > > > Desktop Chrome actually uses two different color orders, BGRA or RGBA, > depending > > on OS. > > Using SkPMColor here means we'll match SkPMColor everwhere, rather than just > > BGRA platforms. > > Linux Mac Win are BGRA order according to Blink image decoders. Android Chrome > is different, RGBA. Color correction is targeted at the desktop machines, no > call for it on mobile devices (uses too battery, screens are not good enough to > warrant color correction). Some other non-mobile Android applications need > color correction though. > > > This is probably not a big deal for speed. It won't make any difference at > all > > on machines > > with SSSE3 (it's a single _mm_shuffle_epi8 either way) but it will for those > > lonely old > > machines with only SSE2 or SSE3, and maybe for ARM devices (I think Clank and > > Chrome OS use > > different orders, so one of them suffers here, don't remember which). > > ARM I think. And agree, we pixel pushers have to swizzle sometimes (for > Android). Sounds right. I bet Chrome OS is BGRA too then. Must be Mac and Android are the oddballs.
Message was sent while issue was closed.
On 2015/08/19 17:59:20, mtklein wrote: > > ARM I think. And agree, we pixel pushers have to swizzle sometimes (for > > Android). > > Sounds right. I bet Chrome OS is BGRA too then. I believe so. Nothing special for them in the Blink image decoders ... > Must be Mac and Android are the oddballs. or for mac either, but there is a special (swizzle decoded pixels to RGBA) for Android. |
