Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1315353006:
refactor parts of SkGr.cpp for use by SkImages (Closed)
Descriptionrefactor parts of SkGr.cpp for use by SkImages
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/43fe6185c5043247c47daa450b015e26d86728fe
Patch Set 1 #Patch Set 2 : had to keep sizes/rowbytes mutable for now :( #Patch Set 3 : #
Total comments: 4
Patch Set 4 : add enums for planes #
Total comments: 4
Patch Set 5 : fix header #
Total comments: 19
Patch Set 6 : address comments, fix leaks #
Total comments: 8
Patch Set 7 : fix nits #
Messages
Total messages: 36 (14 generated)
reed@google.com changed reviewers: + robertphillips@google.com
ptal
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315353006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/20001
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-09-04 20:44 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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...)
reed@google.com changed reviewers: + joshualitt@google.com
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315353006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/40001
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315353006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/60001
some nits https://codereview.chromium.org/1315353006/diff/40001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/40001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:36: planes[0] = (void*)fCachedData->data(); enum kYPlane / kUPlane / kVPlane? https://codereview.chromium.org/1315353006/diff/40001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/40001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:24: virtual bool onGetYUVSizes(SkISize sizes[3]) = 0; constant kNumYUVPlanes? https://codereview.chromium.org/1315353006/diff/40001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/1315353006/diff/40001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:301: GrTexture* stretched = GrCreateTextureForPixels(context, optionalKey, rtDesc, pixelRef, nullptr,0); \n https://codereview.chromium.org/1315353006/diff/40001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:375: if (NULL == data) { nullptr
https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:17: do we need this namespace ? https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:7: GrYUVProvider_DEFINED ?
https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:17: On 2015/09/04 15:20:54, robertphillips wrote: > do we need this namespace ? Well, didn't want to accidentally pollute the global namespace of classes with this local/static one. If I could write static class Foo { ... I'd do that, but I can't :( https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/60001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:7: On 2015/09/04 15:20:54, robertphillips wrote: > GrYUVProvider_DEFINED ? Done.
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315353006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/80001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
https://codereview.chromium.org/1315353006/diff/80001/include/gpu/SkGr.h File include/gpu/SkGr.h (right): https://codereview.chromium.org/1315353006/diff/80001/include/gpu/SkGr.h#newc... include/gpu/SkGr.h:71: // Returns 'kUnknown_GrPixelConfig' if 'data' does not represent a supported compressed format. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:47: size_t totalSize(0); 3 -> kPlaneCount ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:78: SkYUVPlanesCache::Info yuvInfo; 3 -> kPlaneCount ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:91: bool needsExactTexture = This indent seems sub-optimal Also, what is the logic behind this logic ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:127: if (!drawContext) { Won't 'result' leak here ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:16: // This exists b.c. ... ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:32: // returns false when ... ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:300: Add a newline somewhere in here ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:374: SkAutoTUnref<SkData> data(bm.pixelRef()->refEncodedData()); !data or "nullptr == data" ? https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:388: // TODO: this will go away ?
https://codereview.chromium.org/1315353006/diff/80001/include/gpu/SkGr.h File include/gpu/SkGr.h (right): https://codereview.chromium.org/1315353006/diff/80001/include/gpu/SkGr.h#newc... include/gpu/SkGr.h:71: On 2015/09/08 13:34:35, robertphillips wrote: > // Returns 'kUnknown_GrPixelConfig' if 'data' does not represent a supported > compressed format. Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:47: size_t totalSize(0); On 2015/09/08 13:34:35, robertphillips wrote: > 3 -> kPlaneCount ? Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:91: bool needsExactTexture = On 2015/09/08 13:34:35, robertphillips wrote: > This indent seems sub-optimal > > Also, what is the logic behind this logic ? Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:127: if (!drawContext) { On 2015/09/08 13:34:35, robertphillips wrote: > Won't 'result' leak here ? Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:16: On 2015/09/08 13:34:35, robertphillips wrote: > // This exists b.c. ... > > ? Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:32: On 2015/09/08 13:34:35, robertphillips wrote: > // returns false when ... ? Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:300: On 2015/09/08 13:34:35, robertphillips wrote: > Add a newline somewhere in here ? Done. And fixed leak https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:374: SkAutoTUnref<SkData> data(bm.pixelRef()->refEncodedData()); On 2015/09/08 13:34:35, robertphillips wrote: > !data or "nullptr == data" ? Done. https://codereview.chromium.org/1315353006/diff/80001/src/gpu/SkGr.cpp#newcod... src/gpu/SkGr.cpp:388: On 2015/09/08 13:34:35, robertphillips wrote: > // TODO: this will go away ? Done.
The CQ bit was checked by reed@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/1315353006/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm + nits https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:18: namespace { // comment ? https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:117: GrPaint paint; put "SkAutoTUnref<GrFragmentProcessor> yuvToRgbProcessor(" on one line ? This indentation was a shade surprising. https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:21: * typo (abstarct) https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:32: * is true, then the texture will automatically have a key added, so it can be retrived typo (retrived)
https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.cpp File src/gpu/GrYUVProvider.cpp (right): https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:18: namespace { On 2015/09/08 15:04:47, robertphillips wrote: > // comment ? Done. https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.c... src/gpu/GrYUVProvider.cpp:117: GrPaint paint; On 2015/09/08 15:04:47, robertphillips wrote: > put "SkAutoTUnref<GrFragmentProcessor> yuvToRgbProcessor(" on one line ? > This indentation was a shade surprising. Done. https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.h File src/gpu/GrYUVProvider.h (right): https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:21: * On 2015/09/08 15:04:47, robertphillips wrote: > typo (abstarct) Done. https://codereview.chromium.org/1315353006/diff/90001/src/gpu/GrYUVProvider.h... src/gpu/GrYUVProvider.h:32: * is true, then the texture will automatically have a key added, so it can be retrived On 2015/09/08 15:04:47, robertphillips wrote: > typo (retrived) Done.
The CQ bit was checked by reed@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/1315353006/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/110001
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1315353006/#ps110001 (title: "fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315353006/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315353006/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://skia.googlesource.com/skia/+/43fe6185c5043247c47daa450b015e26d86728fe |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
