Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
374743003:
Skia side RGB to YUV gpu conversion (Closed)
DescriptionSkia side RGB to YUV gpu conversion
This code is the one that's currently working in my local chromium build. A few things still need to be addressed and I'll highlight these directly in the code.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/518d83dbc1c899e316e8c896af5defb58b83120f
Patch Set 1 #
Total comments: 9
Patch Set 2 : Some changes to getYUV8Planes() #
Total comments: 14
Patch Set 3 : Fixed comments #
Total comments: 15
Patch Set 4 : Fixed more comments #
Total comments: 9
Patch Set 5 : A few more NULL checks #Patch Set 6 : Fixed comments and added SkImageGenerator unit tests #
Total comments: 2
Patch Set 7 : Ported a minimal set of blink side changes for YUV decoding tests #
Total comments: 18
Patch Set 8 : Back to Patch Set 6 (updated to ToT) #Patch Set 9 : Fixed comments #Patch Set 10 : Fixing more comments #Patch Set 11 : Fixing Windows build #
Messages
Total messages: 38 (0 generated)
Here's my current working implementation on the skia side for the YUV to RGB GPU conversion. Some issues remain, but I'd like to have some input so that we agree on the direction to take. https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerator.h File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerato... include/core/SkImageGenerator.h:121: SkImageInfo fInfo; The SkImageInfo member is required so that onGetYUV8Planes() can call onGetPixels(). I could also take it out of this structure and pass it directly to onGetYUV8Planes() as an extra argument to that function. I put it here because it is not required when the YUV8Planes argument is NULL in onGetYUV8Planes(). https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerato... include/core/SkImageGenerator.h:122: uint8_t* fPixels; The current implementation in Blink makes it very hard to have 3 buffers, so I reverted this back to a single buffer into which the YUV components will be stored sequentially. This can be changed, depending on how we choose to handle changes to ImageFrameGenerator::ExternalMemoryAllocator. https://codereview.chromium.org/374743003/diff/1/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/1/include/core/SkPixelRef.h#ne... include/core/SkPixelRef.h:232: virtual SkImageGenerator* getImageGenerator() const { return NULL; } Since the new API was in SkImageGenerator, I assumed we needed a way to retrieve it from SkPixelRef. https://codereview.chromium.org/374743003/diff/1/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/1/src/gpu/SkGr.cpp#newcode215 src/gpu/SkGr.cpp:215: size_t yuvSize = 4 * ySize; // FIXME: ySize + uSize + vSize; There's still a FIXME here. This allocated memory size is too large. The reason for this is in ImageFrameGenerator::ExternalMemoryAllocator. The current interface uses an SkBitmap to store the data and that bitmap is assumed to always be N32. Unfortunately, JPEGImageDecoder calls ImageFrame::setSize(), which calls ImageFrame::zeroFillPixelData(), which calls SkBitmap::eraseARGB(), so if I allocate less memory, I'll be writing out of bounds. To fix this, I'll need to modify how ImageFrameGenerator::ExternalMemoryAllocator allocates memory, but this will require some discussion. https://codereview.chromium.org/374743003/diff/1/src/gpu/SkGr.cpp#newcode354 src/gpu/SkGr.cpp:354: GrTexture *texture = load_yuv_texture(ctx, params, *bitmap, desc); The "cache" boolean should probably be used here, but it isn't used in load_etc1_texture either, although that could be a mistake.
https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerator.h File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerato... include/core/SkImageGenerator.h:132: bool getYUV8Planes(SkISize* sizes, const YUV8Planes* planes = NULL); Does "planes" have to be a default parameter? It might be slightly clearer if it was explicitly specified at the callsite, to distinguish the "please fill" from "just the sizes, ma'am" flavours. Don't have a strong feeling, just a slight preference against default params.
https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerator.h File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/1/include/core/SkImageGenerato... include/core/SkImageGenerator.h:132: bool getYUV8Planes(SkISize* sizes, const YUV8Planes* planes = NULL); What about struct Plane8 { uint8_t* fPixels; size_t fRowBytes; }; getYUV8Planes(SkISize sizes[3], Plane8 planes[3]); I think adding rowbytes *might* be needed, and certainly doesn't hurt. I don't see any reason to add SkImageInfo to a plane. https://codereview.chromium.org/374743003/diff/1/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/1/include/core/SkPixelRef.h#ne... include/core/SkPixelRef.h:232: virtual SkImageGenerator* getImageGenerator() const { return NULL; } On 2014/07/08 14:42:02, sugoi1 wrote: > Since the new <span id="abbrex-spnAbbr_23889" class="abbrex-Abbr abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span> was in SkImageGenerator, I assumed we needed a way to retrieve > it from SkPixelRef. Hmm, I wonder if its safer to just expose getYUVPlanes() here as well, so we don't require nor expose imagegenerator per-se
I changed the API in SkPixelRef to remove SkImageGenerator and just add a getYUV8Planes() function. I also removed the YUV8Plaves struct and now just pass a void* for the YUV planes storage. Everything else is computed from the void* and the sizes on the blink side. https://codereview.chromium.org/374743003/diff/1/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/1/include/core/SkPixelRef.h#ne... include/core/SkPixelRef.h:232: virtual SkImageGenerator* getImageGenerator() const { return NULL; } On 2014/07/08 15:08:52, reed1 wrote: > On 2014/07/08 14:42:02, sugoi1 wrote: > > Since the new <span id="abbrex-spnAbbr_23889" class="abbrex-Abbr > abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span> was in > SkImageGenerator, I assumed we needed a way to retrieve > > it from SkPixelRef. > > Hmm, I wonder if its safer to just expose getYUVPlanes() here as well, so we > don't require nor expose imagegenerator per-se Done.
https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode199 src/gpu/SkGr.cpp:199: const SkBitmap &bm, GrTextureDesc desc) { style nit: Can you move the & to the left of the space? https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode214 src/gpu/SkGr.cpp:214: // Put the planes into SkBitmap objects Do we really need the intermediate step of going into bitmaps? Can't you just directly create textures on GrContext?
Thanks, I like the api directly on pixelref.
Two API concerns:
1. I think we should pass in rowbytes for each plane, as specified by the
caller.
2. When passing arrays (esp arrays of ptrs) I much prefer that we use array
syntax in the declaration. e.g.
getPlanes(SkISize sizes[3], ...);
https://codereview.chromium.org/374743003/diff/20001/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/20001/include/core/SkPixelRef.... include/core/SkPixelRef.h:238: */ SkISize sizes[3], void* planes[3], int rowBytes[3] ? https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode196 src/gpu/SkGr.cpp:196: You could reduce the indent level of this by reversing most of the tests and returning early https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode198 src/gpu/SkGr.cpp:198: const GrTextureParams* params, const GrTextureDesc& desc ? https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode203 src/gpu/SkGr.cpp:203: SkISize yuvSizes[3]; NULL != pixelRef ? https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode204 src/gpu/SkGr.cpp:204: if (pixelRef && pixelRef->getYUV8Planes(yuvSizes, NULL)) { Allcate -> Allocate ?
https://codereview.chromium.org/374743003/diff/20001/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/20001/include/core/SkPixelRef.... include/core/SkPixelRef.h:238: */ On 2014/07/10 12:47:20, robertphillips wrote: > SkISize sizes[3], void* planes[3], int rowBytes[3] ? Done. https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode196 src/gpu/SkGr.cpp:196: On 2014/07/10 12:47:20, robertphillips wrote: > You could reduce the indent level of this by reversing most of the tests and > returning early Done. https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode198 src/gpu/SkGr.cpp:198: const GrTextureParams* params, On 2014/07/10 12:47:20, robertphillips wrote: > const GrTextureDesc& desc ? Done. https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode199 src/gpu/SkGr.cpp:199: const SkBitmap &bm, GrTextureDesc desc) { On 2014/07/09 20:19:18, bsalomon wrote: > style nit: Can you move the & to the left of the space? Done. https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode203 src/gpu/SkGr.cpp:203: SkISize yuvSizes[3]; On 2014/07/10 12:47:20, robertphillips wrote: > NULL != pixelRef ? Done. https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode204 src/gpu/SkGr.cpp:204: if (pixelRef && pixelRef->getYUV8Planes(yuvSizes, NULL)) { On 2014/07/10 12:47:20, robertphillips wrote: > Allcate -> Allocate ? Done. https://codereview.chromium.org/374743003/diff/20001/src/gpu/SkGr.cpp#newcode214 src/gpu/SkGr.cpp:214: // Put the planes into SkBitmap objects On 2014/07/09 20:19:18, bsalomon wrote: > Do we really need the intermediate step of going into bitmaps? Can't you just > directly create textures on GrContext? Done.
https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... include/core/SkImageGenerator.h:123: * If planes[0] is not NULL, then it should copy the associated Y,U,V data into those planes I presume we mean, "If planes and rowBytes are not NULL" ? https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... include/core/SkImageGenerator.h:127: bool getYUV8Planes(SkISize sizes[3], void* planes[3], int rowBytes[3]); use size_t for rowBytes https://codereview.chromium.org/374743003/diff/40001/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/40001/include/core/SkPixelRef.... include/core/SkPixelRef.h:239: virtual bool getYUV8Planes(SkISize sizes[3], void* planes[3], int rowBytes[3]) { size_t for rowBytes https://codereview.chromium.org/374743003/diff/40001/src/core/SkImageGenerato... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/core/SkImageGenerato... src/core/SkImageGenerator.cpp:60: bool SkImageGenerator::getYUV8Planes(SkISize sizes[3], void* planes[3], int rowBytes[3]) { This is a good place to add generic checks and/or asserts for sane input. e.g. if planes != NULL but rowBytes == NULL if planes != NULL but planes[0] or [1] or [2] is NULL if rowBytes[i] < sizes[i].fWidth if sizes[i].fWidth or fHeight < 0 https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode222 src/gpu/SkGr.cpp:222: if ((NULL == planes[0]) || !pixelRef->getYUV8Planes(yuvSizes, planes, rowBytes)) { I don't think SkAutoMalloc can ever fail, can it?
https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... include/core/SkImageGenerator.h:123: * If planes[0] is not NULL, then it should copy the associated Y,U,V data into those planes On 2014/07/10 17:15:15, reed1 wrote: > I presume we mean, "If planes and rowBytes are not NULL" ? Done. https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... include/core/SkImageGenerator.h:127: bool getYUV8Planes(SkISize sizes[3], void* planes[3], int rowBytes[3]); On 2014/07/10 17:15:14, reed1 wrote: > use size_t for rowBytes Done. https://codereview.chromium.org/374743003/diff/40001/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/40001/include/core/SkPixelRef.... include/core/SkPixelRef.h:239: virtual bool getYUV8Planes(SkISize sizes[3], void* planes[3], int rowBytes[3]) { On 2014/07/10 17:15:15, reed1 wrote: > size_t for rowBytes Done. https://codereview.chromium.org/374743003/diff/40001/src/core/SkImageGenerato... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/core/SkImageGenerato... src/core/SkImageGenerator.cpp:60: bool SkImageGenerator::getYUV8Planes(SkISize sizes[3], void* planes[3], int rowBytes[3]) { On 2014/07/10 17:15:15, reed1 wrote: > This is a good place to add generic checks and/or asserts for sane input. > > e.g. > if planes != NULL but rowBytes == NULL > if planes != NULL but planes[0] or [1] or [2] is NULL > if rowBytes[i] < sizes[i].fWidth > if sizes[i].fWidth or fHeight < 0 > Done. https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode222 src/gpu/SkGr.cpp:222: if ((NULL == planes[0]) || !pixelRef->getYUV8Planes(yuvSizes, planes, rowBytes)) { On 2014/07/10 17:15:15, reed1 wrote: > I don't think SkAutoMalloc can ever fail, can it? I don't know. What happens if we're out of memory?
On 2014/07/10 17:54:25, sugoi1 wrote: > https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... > File include/core/SkImageGenerator.h (right): > > https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... > include/core/SkImageGenerator.h:123: * If planes[0] is not <span id="abbrex-spnAbbr_52540" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span>, then it should > copy the associated Y,U,V data into those planes > On 2014/07/10 17:15:15, reed1 wrote: > > I presume we mean, "If planes and rowBytes are not <span id="abbrex-spnAbbr_52541" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span>" ? > > Done. > > https://codereview.chromium.org/374743003/diff/40001/include/core/SkImageGene... > include/core/SkImageGenerator.h:127: bool getYUV8Planes(SkISize sizes[3], void* > planes[3], int rowBytes[3]); > On 2014/07/10 17:15:14, reed1 wrote: > > use size_t for rowBytes > > Done. > > https://codereview.chromium.org/374743003/diff/40001/include/core/SkPixelRef.h > File include/core/SkPixelRef.h (right): > > https://codereview.chromium.org/374743003/diff/40001/include/core/SkPixelRef.... > include/core/SkPixelRef.h:239: virtual bool getYUV8Planes(SkISize sizes[3], > void* planes[3], int rowBytes[3]) { > On 2014/07/10 17:15:15, reed1 wrote: > > size_t for rowBytes > > Done. > > https://codereview.chromium.org/374743003/diff/40001/src/core/SkImageGenerato... > File src/core/SkImageGenerator.cpp (right): > > https://codereview.chromium.org/374743003/diff/40001/src/core/SkImageGenerato... > src/core/SkImageGenerator.cpp:60: bool SkImageGenerator::getYUV8Planes(SkISize > sizes[3], void* planes[3], int rowBytes[3]) { > On 2014/07/10 17:15:15, reed1 wrote: > > This is a good place to add generic checks and/or asserts for sane input. > > > > e.g. > > if planes != <span id="abbrex-spnAbbr_52542" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span> but rowBytes == <span id="abbrex-spnAbbr_52543" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span> > > if planes != <span id="abbrex-spnAbbr_52544" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span> but planes[0] or [1] or [2] is <span id="abbrex-spnAbbr_52545" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span> > > if rowBytes[i] < sizes[i].fWidth > > if sizes[i].fWidth or fHeight < 0 > > > > Done. > > https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp > File src/gpu/SkGr.cpp (right): > > https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode222 > src/gpu/SkGr.cpp:222: if ((<span id="abbrex-spnAbbr_52546" class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span> == planes[0]) || > !pixelRef->getYUV8Planes(yuvSizes, planes, rowBytes)) { > On 2014/07/10 17:15:15, reed1 wrote: > > I don't think SkAutoMalloc can ever fail, can it? > > I don't know. What happens if we're out of memory? We crash, which is a good thing.
https://codereview.chromium.org/374743003/diff/60001/src/core/SkImageGenerato... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/60001/src/core/SkImageGenerato... src/core/SkImageGenerator.cpp:63: ((NULL != planes[0]) && (NULL != planes[1]) && (NULL != planes[2]) && if planes is NULL, this will crash! We should add some tests for this API, including testing passing NULL where legal (and planes == NULL is legal).
https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp#newcode212 src/gpu/SkGr.cpp:212: for (int i = 0; i < 3; ++i) { Shouldn't all the rowBytes be the same (and equal to the sum of the widths) ? https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp#newcode218 src/gpu/SkGr.cpp:218: planes[1] = (uint8_t*)planes[0] + sizes[0]; Shouldn't this be sizes[0] + sizes[1] ? https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp#newcode231 src/gpu/SkGr.cpp:231: yuvDesc.fHeight = yuvSizes[i].fHeight; Replace "GrTexture* tex =" with "yuvTextures[i].reset(" ?
https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp#newcode218 src/gpu/SkGr.cpp:218: planes[1] = (uint8_t*)planes[0] + sizes[0]; Forget this comment - I overlooked that you use planes[1] here.
sorry meant to send these comments earlier but forgot to publish! https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode232 src/gpu/SkGr.cpp:232: GrTexture* tex = ctx->createUncachedTexture(yuvDesc, planes[i], rowBytes[i]); We should probably use scratch textures here. We'd be able to reuse them when we have a run of bitmap uploads. They can be approx match, I think. https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode261 src/gpu/SkGr.cpp:261: GrContext::AutoClip ac(ctx, r); Maybe ac(ctx, GrContext::AutoClip::kWideOpen_InitialClip) ?
https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode232 src/gpu/SkGr.cpp:232: GrTexture* tex = ctx->createUncachedTexture(yuvDesc, planes[i], rowBytes[i]); On 2014/07/10 18:29:39, bsalomon wrote: > We should probably use scratch textures here. We'd be able to reuse them when we > have a run of bitmap uploads. They can be approx match, I think. Done. https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode261 src/gpu/SkGr.cpp:261: GrContext::AutoClip ac(ctx, r); On 2014/07/10 18:29:39, bsalomon wrote: > Maybe ac(ctx, GrContext::AutoClip::kWideOpen_InitialClip) ? Will that work if I'm rendering a JPEG while rendering a tile that's smaller that the JPEG image? https://codereview.chromium.org/374743003/diff/60001/src/core/SkImageGenerato... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/60001/src/core/SkImageGenerato... src/core/SkImageGenerator.cpp:63: ((NULL != planes[0]) && (NULL != planes[1]) && (NULL != planes[2]) && On 2014/07/10 18:21:40, reed1 wrote: > if planes is NULL, this will crash! > > We should add some tests for this API, including testing passing NULL where > legal (and planes == NULL is legal). Acknowledged. Added the NULL test here, I'll write the unit tests soon. https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp#newcode212 src/gpu/SkGr.cpp:212: for (int i = 0; i < 3; ++i) { On 2014/07/10 18:27:38, robertphillips wrote: > Shouldn't all the rowBytes be the same (and equal to the sum of the widths) ? No, YUV components are stored sequentially, not interleaved, so you can see this as 3 separate mono buffers, stored one after the other. https://codereview.chromium.org/374743003/diff/60001/src/gpu/SkGr.cpp#newcode231 src/gpu/SkGr.cpp:231: yuvDesc.fHeight = yuvSizes[i].fHeight; On 2014/07/10 18:27:38, robertphillips wrote: > Replace "GrTexture* tex =" with "yuvTextures[i].reset(" ? Sure, doesn't hurt.
https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/374743003/diff/40001/src/gpu/SkGr.cpp#newcode261 src/gpu/SkGr.cpp:261: GrContext::AutoClip ac(ctx, r); On 2014/07/10 18:29:39, bsalomon wrote: > Maybe ac(ctx, GrContext::AutoClip::kWideOpen_InitialClip) ? Done. Seems to work properly in all cases I've tested. https://codereview.chromium.org/374743003/diff/60001/src/core/SkImageGenerato... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/60001/src/core/SkImageGenerato... src/core/SkImageGenerator.cpp:63: ((NULL != planes[0]) && (NULL != planes[1]) && (NULL != planes[2]) && On 2014/07/10 18:21:40, reed1 wrote: > if planes is NULL, this will crash! > > We should add some tests for this API, including testing passing NULL where > legal (and planes == NULL is legal). Done.
The Gr code lgtm
Skia has a jpeg decoder. Can we modify it so we can test this on a real generator?
On 2014/07/12 11:15:17, reed1 wrote: > Skia has a jpeg decoder. Can we modify it so we can test this on a real generator? It should be possible, but from what I see, I would need to modify quite a bit of code to do that. Is there any code / test in skia currently using SkDefaultImageDecoderFactory::newDecoder() / SkImageDecoder::Factory()? Is there a (simple) way to add jpeg images to tests? I guess right now I'd have to read the byte stream from a file and copy that into the test, right (like in tests/JpegTest.cpp)? Blink uses DecodingImageGenerator -> ImageFrameGenerator -> ImageDecoder (JPEGImageDecoder), but I guess I'd need something quite different on the skia side, just for testing purposes. What parts of it did you want me to test directly in skia? I can add the full class structure to try and mimic what is done in blink or I could just do tests on the parts of the code already in skia (by making a dummy test PixelRef, for example, which just returns the pre-decoded YUV components of an image, just to test that load_yuv_texture() works ok).
PTAL! I ported some code from my local blink patch that enables YUV decoding into the skia jpeg decoder and added a skia decoding test. I tried to keep changes to a minimum, but it does require quite a bit of changes in the decoder. Please review the decoder (SkImageDecoder_libjpeg.cpp) code carefully (it should save me some time on the blink review later on). Thanks.
Not sure what the best way to expose this to ImageDecoder is, if we should at all. Can we land this CL w/o those changes?
https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageDec... File include/core/SkImageDecoder.h (right): https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageDec... include/core/SkImageDecoder.h:52: virtual bool getImageFormat(SkStream*, SkISize sizes[3]) { This name doesn't seem to match what it does. A better name might be getComponentSizes? https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageDec... include/core/SkImageDecoder.h:58: virtual void setYUVBuffers(void* yuv[3], size_t rowBytes[3]) {} For both of these, why not have two functions: void setYUVBuffers(...) { this->onSetYUVBuffers(...); } virtual void onSetYUVBuffers(...) {} Then we've separated the interface from the implementation (we mostly follow this convention in Skia). https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageGen... include/core/SkImageGenerator.h:120: * If any planes or rowBytes is NULL, this imagegenerator should output the sizes and return either planes or rowBytes? either rowBytes or any entry in planes? https://codereview.chromium.org/374743003/diff/120001/src/core/SkImageGenerat... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/120001/src/core/SkImageGenerat... src/core/SkImageGenerator.cpp:74: // Either we have all planes and rowBytes information or we have none of it The comment in the header seems to say that we can have some but not all. https://codereview.chromium.org/374743003/diff/120001/src/images/SkDecodingIm... File src/images/SkDecodingImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/120001/src/images/SkDecodingIm... src/images/SkDecodingImageGenerator.cpp:227: return onGetPixels(fInfo, NULL, 0, NULL, NULL); this->onGetPixels https://codereview.chromium.org/374743003/diff/120001/src/images/SkImageDecod... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/374743003/diff/120001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:283: printf("YUV 4:%d:%d\n", 4 / h, (v == 1) ? 4 / h : 0); SkDebugf? https://codereview.chromium.org/374743003/diff/120001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:390: (NULL == bm) ? bm->width() : 0, This is backwards. If NULL == bm, you want to return 0. Otherwise you should return bm->width() https://codereview.chromium.org/374743003/diff/120001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:692: return canDecodeToYUV(cinfo) ? decodeToYUV(cinfo) : decodeToBitmap(bm, cinfo, sampler); It seems awkward that if canDecodeToYUV is true we do decode into YUV, with no apparent choice from the caller (I guess that comes earlier whey they called setYUVBuffers). This goes a little deeper though: it's strange that the caller called SkImageDecoder::decode, passing in an SkBitmap, but the implementation ignored that SkBitmap and filled out arrays that were passed in elsewhere. https://codereview.chromium.org/374743003/diff/120001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:844: JDIMENSION scanlinesRead = jpeg_read_raw_data(&cinfo, (JSAMPIMAGE)bufferraw, scanlinesToRead); 100 chars https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... File tests/ImageGeneratorTest.cpp (right): https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... tests/ImageGeneratorTest.cpp:8: #include "Resources.h" Unused? https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... tests/ImageGeneratorTest.cpp:22: ig.getYUV8Planes(sizes, NULL, NULL); Should we REPORTER_ASSERT that these return the values we expect? (I suppose they'll all return false since we are using the base class which won't do anything.) https://codereview.chromium.org/374743003/diff/120001/tests/JpegTest.cpp File tests/JpegTest.cpp (right): https://codereview.chromium.org/374743003/diff/120001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:463: SkAutoTUnref<SkStreamRewindable> stream(SkFrontBufferedStream::Create(&memStream, len)); Why do you need an SkFrontBufferedStream? An SkMemoryStream is already rewindable. https://codereview.chromium.org/374743003/diff/120001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:470: if (pixelsInstalled) { nit: It may be a matter of personal opinion, but you can save the indentation by reversing your if statements: if (!pixelsInstalled) { return; } ... if (!sizesComputed) { return; }
On 2014/07/18 19:26:01, reed1 wrote: > Not sure what the best way to expose this to ImageDecoder is, if we should at all. > > Can we land this CL w/o those changes? Of course. I would much rather land it without the other changes. I added these so that you could have an idea of what they would look like (as requested in comment 19). Are you ok with me landing Patch Set 6?
On 2014/07/21 14:35:18, sugoi1 wrote: > On 2014/07/18 19:26:01, reed1 wrote: > > Not sure what the best way to expose this to ImageDecoder is, if we should at > all. > > > > Can we land this <span id="abbrex-spnAbbr_1298" class="abbrex-Abbr abbrex-Abbr_CL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">CL</span> w/o those changes? > > Of course. I would much rather land it without the other changes. I added these > so that you could have an idea of what they would look like (as requested in > comment 19). > Are you ok with me landing Patch Set 6? lgtm for #6 -- Leon is that CL ok w/ you?
On 2014/07/21 14:45:13, reed1 wrote: > lgtm for #6 -- Leon is that CL ok w/ you? In general, yes, although my comments in patch set 7 (except for the ones in files that aren't a part of 6, obviously) still apply. https://codereview.chromium.org/374743003/diff/100001/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/100001/include/core/SkPixelRef... include/core/SkPixelRef.h:239: virtual bool getYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3]) { Why not follow the convention used in SkImageGenerator (and elsewhere in Skia) and have bool getYUV8Planes() { return this->onGetYUV8Planes(); } virtual bool onGetYUV8Planes();
https://codereview.chromium.org/374743003/diff/100001/include/core/SkPixelRef.h File include/core/SkPixelRef.h (right): https://codereview.chromium.org/374743003/diff/100001/include/core/SkPixelRef... include/core/SkPixelRef.h:239: virtual bool getYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3]) { On 2014/07/21 14:53:00, scroggo wrote: > Why not follow the convention used in SkImageGenerator (and elsewhere in Skia) > and have > > bool getYUV8Planes() { > return this->onGetYUV8Planes(); > } > > virtual bool onGetYUV8Planes(); Done.
Please see my comments in patch set 7: https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageGen... , https://codereview.chromium.org/374743003/diff/120001/src/core/SkImageGenerat... , https://codereview.chromium.org/374743003/diff/120001/src/images/SkDecodingIm... , https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... , https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes...
https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/374743003/diff/120001/include/core/SkImageGen... include/core/SkImageGenerator.h:120: * If any planes or rowBytes is NULL, this imagegenerator should output the sizes and return On 2014/07/18 21:52:40, scroggo wrote: > either planes or rowBytes? > either rowBytes or any entry in planes? Done. https://codereview.chromium.org/374743003/diff/120001/src/core/SkImageGenerat... File src/core/SkImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/120001/src/core/SkImageGenerat... src/core/SkImageGenerator.cpp:74: // Either we have all planes and rowBytes information or we have none of it On 2014/07/18 21:52:40, scroggo wrote: > The comment in the header seems to say that we can have some but not all. I'll change the header https://codereview.chromium.org/374743003/diff/120001/src/images/SkDecodingIm... File src/images/SkDecodingImageGenerator.cpp (right): https://codereview.chromium.org/374743003/diff/120001/src/images/SkDecodingIm... src/images/SkDecodingImageGenerator.cpp:227: return onGetPixels(fInfo, NULL, 0, NULL, NULL); On 2014/07/18 21:52:41, scroggo wrote: > this->onGetPixels Acknowledged. https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... File tests/ImageGeneratorTest.cpp (right): https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... tests/ImageGeneratorTest.cpp:8: #include "Resources.h" On 2014/07/18 21:52:41, scroggo wrote: > Unused? Removed https://codereview.chromium.org/374743003/diff/120001/tests/ImageGeneratorTes... tests/ImageGeneratorTest.cpp:22: ig.getYUV8Planes(sizes, NULL, NULL); On 2014/07/18 21:52:41, scroggo wrote: > Should we REPORTER_ASSERT that these return the values we expect? (I suppose > they'll all return false since we are using the base class which won't do > anything.) This test is just to make sure that no valid combination of entries can make it crash. The return value, which will indeed always be false, doesn't really matter.
lgtm
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/374743003/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
The CQ bit was unchecked by sugoi@chromium.org
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/374743003/200001
Message was sent while issue was closed.
Change committed as 518d83dbc1c899e316e8c896af5defb58b83120f |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
