Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1923533002:
Remove redundant tests to make DM a little faster (Closed)
DescriptionRemove redundant tests to make DM a little faster
This should help with some of the slow down caused by
https://codereview.chromium.org/1907593004.
BUG=skia:5181
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1923533002
Committed: https://skia.googlesource.com/skia/+/f3dc18851411c4a0f1d5c3ecc17353042e6cd440
Patch Set 1 #
Total comments: 5
Patch Set 2 : Test all alpha modes #Messages
Total messages: 22 (10 generated)
Description was changed from ========== Make DM run a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia: ========== to ========== Make DM run a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Make DM run a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Remove redundant tests to make DM a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + benjaminwagner@google.com, scroggo@google.com, stephana@google.com
Add the bug to the commit message? I think I've suggested this before, but what if we only triggered image decoding tests if the image decoding code changed? (or we're running on a new device) I'm sure we're not set up to do that now, but maybe the work to set that up would be worth it? There are some pieces that depend on other code (e.g. opts), but I think most Skia changes won't affect image decoding. https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp#newcode550 dm/DM.cpp:550: // Only test kNonNative_DstColorType when the alpha type is premul and there is I found this a little bit confusing. I thought you meant when the image's alpha type is premul, leaving me confused why we didn't care to test an opaque image with both byte orders. Here's my understanding: We normally test decoding an image while requesting premul alpha type and unpremul. If the image is opaque, we also request opaque. This prevents us from doing the second and third ones. Is that correct? I understand skipping opaque, which would follow the same code path, but unpremul will take a different path, so I think that might be interesting. As for the scale, I could imagine that our sampling code is wrong for one byte order, so I could imagine it being useful to test a scale. https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp#newcode590 dm/DM.cpp:590: // alpha types or additional sample sizes. Why not for additional sample sizes? Won't we be exercising different code?
On 2016/04/26 at 13:22:33, scroggo wrote: > I think I've suggested this before, but what if we only triggered image decoding tests if the image decoding code changed? (or we're running on a new device) I'm sure we're not set up to do that now, but maybe the work to set that up would be worth it? > > There are some pieces that depend on other code (e.g. opts), but I think most Skia changes won't affect image decoding. IMHO, getting the above working would be much more complicated than adding a new bot for image decoding tests, especially if you don't need to test on specific hardware.
On 2016/04/26 13:50:39, Ben Wagner wrote: > On 2016/04/26 at 13:22:33, scroggo wrote: > > I think I've suggested this before, but what if we only triggered image > decoding tests if the image decoding code changed? (or we're running on a new > device) I'm sure we're not set up to do that now, but maybe the work to set that > up would be worth it? > > > > There are some pieces that depend on other code (e.g. opts), but I think most > Skia changes won't affect image decoding. > > IMHO, getting the above working would be much more complicated than adding a new > bot for image decoding tests, especially if you don't need to test on specific > hardware. That's an interesting idea, too. But we do want specific hardware.
Description was changed from ========== Remove redundant tests to make DM a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Remove redundant tests to make DM a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia:5181 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
"""Add the bug to the commit message.""" Done. """> > I think I've suggested this before, but what if we only triggered image > decoding tests if the image decoding code changed? (or we're running on a new > device) I'm sure we're not set up to do that now, but maybe the work to set that > up would be worth it? > > > > There are some pieces that depend on other code (e.g. opts), but I think most > Skia changes won't affect image decoding. > > IMHO, getting the above working would be much more complicated than adding a new > bot for image decoding tests, especially if you don't need to test on specific > hardware. That's an interesting idea, too. But we do want specific hardware.""" To add an additional idea, I think we could refactor/rewrite DM.cpp to only test "interesting" cases rather than a giant cross product of cases. But this is a bit dangerous, as Leon just caught me trying to delete useful tests... https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp#newcode550 dm/DM.cpp:550: // Only test kNonNative_DstColorType when the alpha type is premul and there is On 2016/04/26 13:22:33, scroggo wrote: > I found this a little bit confusing. I thought you meant when the image's alpha > type is premul, leaving me confused why we didn't care to test an opaque image > with both byte orders. Here's my understanding: > > We normally test decoding an image while requesting premul alpha type and > unpremul. > If the image is opaque, we also request opaque. > This prevents us from doing the second and third ones. > > Is that correct? Yes you're correct. You make a good point that unpremul swizzles differently than premul. We should test both. I think we can safely drop opaque, decoding opaque images to premul/unpremul/opaque will exercise the same code. IMO, this is sufficiently confusing that I'd rather back off on disabling alpha modes. I'd rather come up with a long term fix than pile on these hacks. > > I understand skipping opaque, which would follow the same code path, but > unpremul will take a different path, so I think that might be interesting. > > As for the scale, I could imagine that our sampling code is wrong for one byte > order, so I could imagine it being useful to test a scale. Yes. We should test our sampling code. "Native" scales don't test sampling though, they test scaling in libjpeg-turbo/libwebp. https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp#newcode590 dm/DM.cpp:590: // alpha types or additional sample sizes. On 2016/04/26 13:22:33, scroggo wrote: > Why not for additional sample sizes? Won't we be exercising different code? Sampling vs not-sampling will exercise different code. But sampleSize=2 vs sampleSize=3,4,5,6,7,8 all exercise the same code. I thought it would be good enough to exercise the sampling code once (with sampleSize = 2). What do you think? I'm adding sampleSize 3 as well for a little additional coverage.
lgtm https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1923533002/diff/1/dm/DM.cpp#newcode590 dm/DM.cpp:590: // alpha types or additional sample sizes. On 2016/04/26 15:00:54, msarett wrote: > On 2016/04/26 13:22:33, scroggo wrote: > > Why not for additional sample sizes? Won't we be exercising different code? > > Sampling vs not-sampling will exercise different code. But sampleSize=2 vs > sampleSize=3,4,5,6,7,8 all exercise the same code. I thought it would be good > enough to exercise the sampling code once (with sampleSize = 2). What do you > think? > > I'm adding sampleSize 3 as well for a little additional coverage. sgtm
The CQ bit was checked by msarett@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/1923533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923533002/20001
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...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) 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-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) 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 msarett@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/1923533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923533002/20001
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 msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923533002/20001
Message was sent while issue was closed.
Description was changed from ========== Remove redundant tests to make DM a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia:5181 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Remove redundant tests to make DM a little faster This should help with some of the slow down caused by https://codereview.chromium.org/1907593004. BUG=skia:5181 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f3dc18851411c4a0f1d5c3ecc17353042e6cd440 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/f3dc18851411c4a0f1d5c3ecc17353042e6cd440 |
