Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1240093004:
SkPictureImageGenerator (Closed)
DescriptionSkPictureImageGenerator
R=reed@google.com
Committed: https://skia.googlesource.com/skia/+/1dedc3d2c00468d9b4d0f0a8e69cb56acd08698f
Patch Set 1 #
Total comments: 4
Patch Set 2 : drop redundant fInfo #Patch Set 3 : correct installPixel result test #Patch Set 4 : drop redundant assert #Patch Set 5 : review comments #Patch Set 6 : added GM #
Total comments: 8
Patch Set 7 : Win warnings #Patch Set 8 : review comments #Patch Set 9 : rebased, review nit #Patch Set 10 : build fix #
Messages
Total messages: 45 (18 generated)
WIP. At this point I can only guarantee that it builds :)
reed@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
We should consider allowing the client to tell us that the resulting image will be opaque (but not premul -vs- unpremul). Just a bool seems sufficient. https://codereview.chromium.org/1240093004/diff/1/src/core/SkPictureImageGene... File src/core/SkPictureImageGenerator.h (right): https://codereview.chromium.org/1240093004/diff/1/src/core/SkPictureImageGene... src/core/SkPictureImageGenerator.h:19: static SkImageGenerator* create(const SkISize&, const SkPicture*, const SkMatrix*, Create https://codereview.chromium.org/1240093004/diff/1/src/core/SkPictureImageGene... src/core/SkPictureImageGenerator.h:31: SkTLazy<SkMatrix> fMatrix; No real reason to make matrix lazy -- it could just be identity if the caller didn't specify one The paint, however, is important to know if it was NULL or not, since drawPicture behaves very differently if it is non-null (i.e. it draws into a layer if there is a paint).
Now on to actually trying this out... https://codereview.chromium.org/1240093004/diff/1/src/core/SkPictureImageGene... File src/core/SkPictureImageGenerator.h (right): https://codereview.chromium.org/1240093004/diff/1/src/core/SkPictureImageGene... src/core/SkPictureImageGenerator.h:19: static SkImageGenerator* create(const SkISize&, const SkPicture*, const SkMatrix*, On 2015/07/17 18:25:42, reed1 wrote: > Create Done. https://codereview.chromium.org/1240093004/diff/1/src/core/SkPictureImageGene... src/core/SkPictureImageGenerator.h:31: SkTLazy<SkMatrix> fMatrix; On 2015/07/17 18:25:42, reed1 wrote: > No real reason to make matrix lazy -- it could just be identity if the caller > didn't specify one > > The paint, however, is important to know if it was NULL or not, since > drawPicture behaves very differently if it is non-null (i.e. it draws into a > layer if there is a paint). Done.
The CQ bit was checked by fmalita@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/1240093004/100001
New GM: http://i.imgur.com/mxbdwBV.png PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by fmalita@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/1240093004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... File gm/pictureimagegenerator.cpp (right): https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... gm/pictureimagegenerator.cpp:20: static ? no namespace ? https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... gm/pictureimagegenerator.cpp:100: // This really interesting GM exercises ... In particular why are the scales and opacity interesting ? https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... gm/pictureimagegenerator.cpp:115: SkPictureRecorder recorder; Can we make 'fRect' just be local to this method ? Maybe define 200 & 100 to be kPictureWidth & kPictureHeight at class scope? https://codereview.chromium.org/1240093004/diff/100001/src/core/SkPictureImag... File src/core/SkPictureImageGenerator.cpp (right): https://codereview.chromium.org/1240093004/diff/100001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:39: So is there no way to have this work in GPU land?
https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... File gm/pictureimagegenerator.cpp (right): https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... gm/pictureimagegenerator.cpp:20: On 2015/07/20 17:32:43, robertphillips wrote: > static ? no namespace ? Done. https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... gm/pictureimagegenerator.cpp:100: On 2015/07/20 17:32:43, robertphillips wrote: > // This really interesting GM exercises ... > In particular why are the scales and opacity interesting > > ? Done. https://codereview.chromium.org/1240093004/diff/100001/gm/pictureimagegenerat... gm/pictureimagegenerator.cpp:115: SkPictureRecorder recorder; On 2015/07/20 17:32:43, robertphillips wrote: > Can we make 'fRect' just be local to this method ? > Maybe define 200 & 100 to be kPictureWidth & kPictureHeight at class scope? Done. https://codereview.chromium.org/1240093004/diff/100001/src/core/SkPictureImag... File src/core/SkPictureImageGenerator.cpp (right): https://codereview.chromium.org/1240093004/diff/100001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:39: On 2015/07/20 17:32:43, robertphillips wrote: > So is there no way to have this work in GPU land? Not at this point :( It would be super nice to have that, but, based on my limited understanding of SkImageGenerator, the machinery needed to support it is missing. I recall Mike having some ideas for adding a GPU path to the API, so I'm optimistic.
The CQ bit was checked by fmalita@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/1240093004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nit: seems like we don't need a separate header for the subclass, as it can be inline with the impl in the same file as the Create impl. lgtm
On 2015/07/27 20:24:59, reed1 wrote: > nit: seems like we don't need a separate header for the subclass, as it can be > inline with the impl in the same file as the Create impl. Done.
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1240093004/#ps160001 (title: "rebased, review nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240093004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1240093004/160001
The CQ bit was unchecked by commit-bot@chromium.org
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...) 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...)
forget to add a private header?
On 2015/08/04 17:39:27, reed1 wrote: > forget to add a private header? Hmm, I had just inlined the class in the cpp file... maybe I forgot some gyp references to the header.
On 2015/08/04 17:43:01, f(malita) wrote: > On 2015/08/04 17:39:27, reed1 wrote: > > forget to add a private header? > > Hmm, I had just inlined the class in the cpp file... maybe I forgot some gyp > references to the header. Removed the old header reference from GM.
The CQ bit was checked by fmalita@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/1240093004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1240093004/180001
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-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 fmalita@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/1240093004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1240093004/180001
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-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 fmalita@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/1240093004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1240093004/180001
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-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-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 fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1240093004/#ps180001 (title: "build fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240093004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1240093004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/1dedc3d2c00468d9b4d0f0a8e69cb56acd08698f |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
