Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1549473003:
Add getYUV8Planes() API to SkCodec (Closed)
DescriptionAdd getYUV8Planes() API to SkCodec
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1549473003
Committed: https://skia.googlesource.com/skia/+/b714fb0199e8727ef2b6cddbee7eba6046f01554
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Add support for progressive images #Patch Set 3 : Remove random extra file :) #
Total comments: 52
Patch Set 4 : queryYUV8() returns size, widthBytes, and colorSpace #
Total comments: 7
Patch Set 5 : Respond to nits I missed #Patch Set 6 : Rebase #Patch Set 7 : Add a test to DM #
Total comments: 19
Patch Set 8 : Response to comments #
Total comments: 6
Patch Set 9 : Combined the structs #
Total comments: 9
Patch Set 10 : Less comments #Patch Set 11 : Fix return value #Patch Set 12 : #
Depends on Patchset: Messages
Total messages: 45 (15 generated)
Description was changed from ========== Add getYUV8Planes() API to SkCodec BUG=skia: ========== to ========== Add getYUV8Planes() API to SkCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + bsalomon@google.com, reed@google.com, scroggo@google.com
This is different than the API used throughout the rest of
Skia.
SkImageDecoder Example:
SkISize sizes[3];
// Get the size of the planes
decoder->getYUV8Planes(sizes, NULL, NULL ...)
// Allocate memory etc.
decoder->getYUV8Planes(sizes, pixels, rowsBytes ...)
SkCodec Example:
SkISize sizes[3];
// Get the size of the planes
codec->getYUV8Sizes(sizes)
// Allocate memory etc.
codec->getYUV8Planes(sizes, pixels, rowsBytes ...)
Regardless, there is an issue with both APIs that I think
is kind of tricky. libjpeg/libjpeg-turbo requires that
both the width and height of memory be padded to a multiple
of 8.
SkImageDecoder handles this by always recommending width
and height as multiples of 8. Then, after completing the
decode, it will silently modify the SkISize* to have the
true values of the width and height.
SkCodec is slightly cleverer than SkImageDecoder on the
height. We can recommend the true height (and avoid
needing padding rows) by pointing all of the extra rowptrs
to a garbage row. SkImageDecoder actually does this - it's
just overkill because they are already allocating the extra
memory.
The width is trickier because all of the rows in the
contiguous block of memory must be padded. One possible
solution is to change getYUV8Size() to recommend values
for "size" and "rowBytes".
This would work up until the last row of the image (which will not necessarily
be padded even if rowBytes is larger
than the width). We could fix the issue with the last row
by decoding into a temporary buffer, and then memcpying.
Amusingly, SkImageDecoder does this, even though it is
overkill in the case where enough memory has been allocated
(by recommending the padded width). I didn't do this
because it makes my code uglier and slower - though I think
this may be the best approach.
Right now my solution is as follows:
(1) Recommend the padded width and the true height.
(2) There is no extra work to be done in onGetYUV8Planes(),
since dummy rows can be used to pad the height, and
the width is already padded.
(3) Sneakily update the width after the decode is over,
same as SkImageDecoder does (this is the part I don't
like).
Brian, maybe we should talk about how the gpu uses the
sizes of the planes? That might help us decide how best to
set up the API.
https://codereview.chromium.org/1549473003/diff/20001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):
https://codereview.chromium.org/1549473003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:496: virtual SkISize onGetScaledDimensions(float
/*desiredScale*/) const {
Just changing the comments here to be consistent with the rest of the file.
https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):
https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:471: // removing this check.
Chromium supports progressive images (in a different way than we will).
SkImageDecoder looks like it is trying to disable progressive images. I think
it will disable them most of the time and probably behave oddly when it fails to
disable them.
https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:479: // only the common cases.
Chromium and SkImageDecoder only support these common cases.
https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:632: *colorSpace = kJPEG_SkYUVColorSpace;
Can someone comment on if/why this is useful? Would it instead/also be
interesting to report the type of YUV subsampling (ex: 444, 422, 420...)?
https://codereview.chromium.org/1549473003/diff/20001/tests/JpegTest.cpp
File tests/JpegTest.cpp (right):
https://codereview.chromium.org/1549473003/diff/20001/tests/JpegTest.cpp#newc...
tests/JpegTest.cpp:568: DEF_TEST(Jpeg_YUV_Codec, r) {
It would be nice to add a visual test too. I'm not sure how to add a gm/image
that draws YUV. Maybe it's fine to just wait until we replace
SkImageDecoderGenerator?
On 2015/12/22 21:01:36, msarett wrote: > This is different than the API used throughout the rest of > Skia. > > SkImageDecoder Example: > SkISize sizes[3]; > // Get the size of the planes > decoder->getYUV8Planes(sizes, NULL, NULL ...) > // Allocate memory etc. > decoder->getYUV8Planes(sizes, pixels, rowsBytes ...) > > SkCodec Example: > SkISize sizes[3]; > // Get the size of the planes > codec->getYUV8Sizes(sizes) > // Allocate memory etc. > codec->getYUV8Planes(sizes, pixels, rowsBytes ...) > > Regardless, there is an issue with both APIs that I think > is kind of tricky. libjpeg/libjpeg-turbo requires that > both the width and height of memory be padded to a multiple > of 8. > > SkImageDecoder handles this by always recommending width > and height as multiples of 8. Then, after completing the > decode, it will silently modify the SkISize* to have the > true values of the width and height. > > SkCodec is slightly cleverer than SkImageDecoder on the > height. We can recommend the true height (and avoid > needing padding rows) by pointing all of the extra rowptrs > to a garbage row. SkImageDecoder actually does this - it's > just overkill because they are already allocating the extra > memory. > > The width is trickier because all of the rows in the > contiguous block of memory must be padded. One possible > solution is to change getYUV8Size() to recommend values > for "size" and "rowBytes". > > This would work up until the last row of the image (which will not necessarily > be padded even if rowBytes is larger > than the width). We could fix the issue with the last row > by decoding into a temporary buffer, and then memcpying. > Amusingly, SkImageDecoder does this, even though it is > overkill in the case where enough memory has been allocated > (by recommending the padded width). I didn't do this > because it makes my code uglier and slower - though I think > this may be the best approach. > > Right now my solution is as follows: > (1) Recommend the padded width and the true height. > (2) There is no extra work to be done in onGetYUV8Planes(), > since dummy rows can be used to pad the height, and > the width is already padded. > (3) Sneakily update the width after the decode is over, > same as SkImageDecoder does (this is the part I don't > like). > > Brian, maybe we should talk about how the gpu uses the > sizes of the planes? That might help us decide how best to > set up the API. We upload each plane to a texture. We give OpenGL the pointer to the plane memory, the dimensions, and rowbytes and it sucks the data into the texture. I can look into it if it is helpful, but I believe that if the last row's memory is only valid up to width and not up to rowbytes (e.g. allocated size is [rowBytes*(h-1) + width]) that OpenGL will not try to read the invalid area between width and rowbytes on the last row. > > https://codereview.chromium.org/1549473003/diff/20001/include/codec/SkCodec.h > File include/codec/SkCodec.h (right): > > https://codereview.chromium.org/1549473003/diff/20001/include/codec/SkCodec.h... > include/codec/SkCodec.h:496: virtual SkISize onGetScaledDimensions(float > /*desiredScale*/) const { > Just changing the comments here to be consistent with the rest of the file. > > https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.cpp > File src/codec/SkJpegCodec.cpp (right): > > https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:471: // removing this check. > Chromium supports progressive images (in a different way than we will). > > SkImageDecoder looks like it is trying to disable progressive images. I think > it will disable them most of the time and probably behave oddly when it fails to > disable them. > > https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:479: // only the common cases. > Chromium and SkImageDecoder only support these common cases. > > https://codereview.chromium.org/1549473003/diff/20001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:632: *colorSpace = kJPEG_SkYUVColorSpace; > Can someone comment on if/why this is useful? Would it instead/also be > interesting to report the type of YUV subsampling (ex: 444, 422, 420...)? > > https://codereview.chromium.org/1549473003/diff/20001/tests/JpegTest.cpp > File tests/JpegTest.cpp (right): > > https://codereview.chromium.org/1549473003/diff/20001/tests/JpegTest.cpp#newc... > tests/JpegTest.cpp:568: DEF_TEST(Jpeg_YUV_Codec, r) { > It would be nice to add a visual test too. I'm not sure how to add a gm/image > that draws YUV. Maybe it's fine to just wait until we replace > SkImageDecoderGenerator?
https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:305: Result getYUV8Planes(SkISize sizes[3], void* pixels[3], size_t rowBytes[3], can sizes[] be const? Seems much simplier if they are.
In addition, should we update DM to test getYUV? Or leave that to a separate change? https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:284: bool getYUV8Sizes(SkISize sizes[3]) const { crrev.com/863053002 proposed suggesting both a logical size and an optimal size. IIRC the implementation in SkImageDecoder is a little wacky (sizes means one thing sometimes and another other times), and that was part of cleaning it up. Would it make sense to include that option here? https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:292: /** nit: Typically the first line will describe generally what the method does. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:294: * @param pixels Memory for each of the Y, U, and V planes Maybe this is just due to my lack of knowledge about the YUV data, but it might be nice to describe how big we expect the memory to be; at least how many bytes per logical pixel? https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:296: * @param colorSpace If nullptr, this parameter is ignored. Otherwise, Do we need this here? In https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene... we discussed returning it in the query step (named queryYUV8 in that CL; getYUV8Sizes here). It looks like since we only ever return one answer, we could put it in the query step for now, and then revisit if necessary? https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:305: Result getYUV8Planes(SkISize sizes[3], void* pixels[3], size_t rowBytes[3], On 2016/01/04 15:50:55, reed1 wrote: > can sizes[] be const? Seems much simplier if they are. Looks like rowBytes can also be const? Alternatively, maybe we could use a struct similar to crrev.com/1396323007? https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:527: virtual Result onGetYUV8Planes(SkISize* /*sizes*/, void** /*pixels*/, size_t* /*rowBytes*/, Why does this use different notations from getYUV8Planes? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:461: SkASSERT(8 == DCTSIZE); Can this be a compile time assert (i.e. static_assert)? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:474: bool unitUV = (1 == dinfo->comp_info[1].h_samp_factor) && Instead of creating this boolean that is just read at the end, what if we instead said: if (!<unitUV>) { return false; } ... return <commonY> https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:481: // TODO: Chromium reports the YUV subsampling factors to the client. nit: blank line before comment. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:483: // Could it be useful to do the same here? If we're going to replace/merge with Chromium, it would make sense to report the same. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:513: fDecoderMgr->returnFailure("onGetYUV8Sizes", kInvalidInput); return https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:551: yuv[0] = &rowptrs[0]; // Y rows (8 or 16) nit: Line up these comments? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:558: rowptrs[i] = (JSAMPROW) SkTAddOffset<void>(pixels[0], i * rowBytes[0]); I think part of the point of SkTAddOffset is that it does the cast for you? Can't this be rowptrs[i] = SkTAddOffset<JSAMPROW>(...); https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:561: rowptrs[i + 16] =(JSAMPROW) SkTAddOffset<void>(pixels[1], i * rowBytes[1]); nit: space between "=" and the rhs. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:567: int blockIncrementU = 8 * rowBytes[1]; Do all these "8"s refer to DCTSIZE? Should we use that constant instead? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:571: // We intentionally round down here. As a final step, we may need to decode one Why do we intentionally round down? And why might we need to decode one more partial block? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:573: int numIters = dinfo->output_height / numRowsPerBlock; can be const? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:575: while (iters < numIters) { It looks like iters is only used inside the loop. Can this be a for loop? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:610: JDIMENSION linesRead = jpeg_read_raw_data(dinfo, yuv, numRowsPerBlock); Should this use |remainingRows| instead of |numRowsPerBlock|? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:614: return kIncompleteInput; Doesn't kIncompleteInput mean (in getPixels etc) that we do display what we can and fill the rest? It seems like these should be kInvalidInput until we implement filling? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:618: // FIXME: This is ugly. We should find a solution where the input memory is Does making the query call return both the logical and optimal sizes (as in crrev.com/863053002) help with this problem? https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp File tests/JpegTest.cpp (right): https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:521: if (expectedSizes) { Instead of this if/else block, how do you feel about REPORTER_ASSERT(reporter, (expectedSizes == nullptr) == !success); or something like that? https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:531: REPORTER_ASSERT(reporter, expectedSizes[0].width() == sizes[0].width()); Alternatively, you could do a memcmp? I don't have a strong preference, but that would be approximately one line (and I think you do this elsewhere). https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:538: // Calculate rowBytes for the YUV decode nit: It seems like no "calculation" is done. Looks like you just set it to the width?
Just responding to the API comments. I think we ought to nail that down first. As far as DM testing, I am in favor of it. I just need to figure out how to draw YUV planes to a canvas. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:284: bool getYUV8Sizes(SkISize sizes[3]) const { On 2016/01/04 18:29:15, scroggo wrote: > crrev.com/863053002 proposed suggesting both a logical size and an optimal size. > IIRC the implementation in SkImageDecoder is a little wacky (sizes means one > thing sometimes and another other times), and that was part of cleaning it up. > Would it make sense to include that option here? I think this is the hard question for this API. I commented on this a bit when I published the first Patch Set, but we might as well move the discussion here. The jpeg library (whichever one it is) requires that the width and height of the input memory be padded to a multiple of 8. Thus, SkImageDecoder makes a distinction between AllocatedSize and ActualSize. It will suggest AllocatedSize, and then, after it finishes the decode, it will change the size ptr to the ActualSize. Chromium does something very similar. This is actually unnecessary for the height. We can always use the ActualSize. The jpeg library takes as input an array of row ptrs, so if we need padding, we can trick it by passing in ptrs to a garbage row. The width is what causes us a problem. Here a few suggestions. I imagine that there are more solutions: (1) When the client calls getYUV8Sizes(), we return the AllocatedWidth. After completing the decode in getYUV8Planes(), set the width to ActualWidth. This is what SkImageDecoder does. And this is currently what my implementation does. In this implementation, 'sizes' cannot be const but 'rowBytes' can be const. (2) Allow the client to query for 'ActualWidth' and 'AllocatedWidth' (or logical size and optimal size, as you call them). If they call getYUV8Planes() with 'ActualWidth', we will be able to complete the decode slowly using memcpys. If they call with 'AllocatedWidth', we can decode directly into their memory. I still think we would need to set the width to 'ActualWidth' at the end of this decode. Which I still think is messy and confusing. (3) Make the query function suggest a value for "sizes" and "rowBytes". Clients must provide memory with enough rowBytes for us to perform the decode. This works until the last row (which will not necessarily be padded). We could get around this with a memcpy() (which I don't like because it is slow). Or we could require that the last row also be padded (which is weird because we are changing the definition of rowBytes). But at least sizes and rowBytes could both be const. I think (3) is the best, but we still have to sort out what to do with the bottom row. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:294: * @param pixels Memory for each of the Y, U, and V planes On 2016/01/04 18:29:15, scroggo wrote: > Maybe this is just due to my lack of knowledge about the YUV data, but it might > be nice to describe how big we expect the memory to be; at least how many bytes > per logical pixel? Will add comments. Each Y, U, or V sample is a single byte. Generally, the size of the Y plane will equal the size of the image, while the U and V planes may be downsampled further. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:296: * @param colorSpace If nullptr, this parameter is ignored. Otherwise, On 2016/01/04 18:29:15, scroggo wrote: > Do we need this here? In > https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene... > we discussed returning it in the query step (named queryYUV8 in that CL; > getYUV8Sizes here). It looks like since we only ever return one answer, we could > put it in the query step for now, and then revisit if necessary? Happy to add this to the query step. What is this for? It seems to me like this provides no useful information? Of course, I have no idea what kRec601_SkYUVColorSpace is? IMO, it could possibly be more useful to report the type of YUV color space more specifically (ex: 444, 422, ...). See: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But it looks like no one uses this information either. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:305: Result getYUV8Planes(SkISize sizes[3], void* pixels[3], size_t rowBytes[3], On 2016/01/04 18:29:15, scroggo wrote: > On 2016/01/04 15:50:55, reed1 wrote: > > can sizes[] be const? Seems much simplier if they are. > Depends on how we choose to deal with AllocatedSize vs. ActualSize. Ideally, we will make it const. See the discussion above regarding some different options. > Looks like rowBytes can also be const? > Yes. > Alternatively, maybe we could use a struct similar to crrev.com/1396323007? +1. I'm happy to use structs for "sizes" and "rowBytes".
Patchset #4 (id:80001) has been deleted
PTAL. All the parameters to getYUV8Planes() are const except for void* planes[3]. "In addition, should we update DM to test getYUV? Or leave that to a separate change?" I'm working on this. Will probably make it a separate change unless you feel strongly. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:284: bool getYUV8Sizes(SkISize sizes[3]) const { On 2016/01/04 21:46:46, msarett wrote: > On 2016/01/04 18:29:15, scroggo wrote: > > crrev.com/863053002 proposed suggesting both a logical size and an optimal > size. > > IIRC the implementation in SkImageDecoder is a little wacky (sizes means one > > thing sometimes and another other times), and that was part of cleaning it up. > > Would it make sense to include that option here? > > I think this is the hard question for this API. I commented on this a bit when > I published the first Patch Set, but we might as well move the discussion here. > > The jpeg library (whichever one it is) requires that the width and height of the > input memory be padded to a multiple of 8. Thus, SkImageDecoder makes a > distinction between AllocatedSize and ActualSize. > > It will suggest AllocatedSize, and then, after it finishes the decode, it will > change the size ptr to the ActualSize. Chromium does something very similar. > > This is actually unnecessary for the height. We can always use the ActualSize. > The jpeg library takes as input an array of row ptrs, so if we need padding, we > can trick it by passing in ptrs to a garbage row. > > The width is what causes us a problem. Here a few suggestions. I imagine that > there are more solutions: > (1) When the client calls getYUV8Sizes(), we return the AllocatedWidth. After > completing the decode in getYUV8Planes(), set the width to ActualWidth. This is > what SkImageDecoder does. And this is currently what my implementation does. > In this implementation, 'sizes' cannot be const but 'rowBytes' can be const. > (2) Allow the client to query for 'ActualWidth' and 'AllocatedWidth' (or logical > size and optimal size, as you call them). If they call getYUV8Planes() with > 'ActualWidth', we will be able to complete the decode slowly using memcpys. If > they call with 'AllocatedWidth', we can decode directly into their memory. I > still think we would need to set the width to 'ActualWidth' at the end of this > decode. Which I still think is messy and confusing. > (3) Make the query function suggest a value for "sizes" and "rowBytes". Clients > must provide memory with enough rowBytes for us to perform the decode. This > works until the last row (which will not necessarily be padded). We could get > around this with a memcpy() (which I don't like because it is slow). Or we > could require that the last row also be padded (which is weird because we are > changing the definition of rowBytes). But at least sizes and rowBytes could > both be const. > > I think (3) is the best, but we still have to sort out what to do with the > bottom row. I have invented "widthBytes" which almost means the same thing as "rowBytes". Please see the latest Patch Set 4. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:292: /** On 2016/01/04 18:29:15, scroggo wrote: > nit: Typically the first line will describe generally what the method does. Done. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:294: * @param pixels Memory for each of the Y, U, and V planes On 2016/01/04 21:46:46, msarett wrote: > On 2016/01/04 18:29:15, scroggo wrote: > > Maybe this is just due to my lack of knowledge about the YUV data, but it > might > > be nice to describe how big we expect the memory to be; at least how many > bytes > > per logical pixel? > > Will add comments. Each Y, U, or V sample is a single byte. Generally, the > size of the Y plane will equal the size of the image, while the U and V planes > may be downsampled further. Added more detail. https://codereview.chromium.org/1549473003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:305: Result getYUV8Planes(SkISize sizes[3], void* pixels[3], size_t rowBytes[3], On 2016/01/04 21:46:46, msarett wrote: > On 2016/01/04 18:29:15, scroggo wrote: > > On 2016/01/04 15:50:55, reed1 wrote: > > > can sizes[] be const? Seems much simplier if they are. > > > > Depends on how we choose to deal with AllocatedSize vs. ActualSize. Ideally, we > will make it const. See the discussion above regarding some different options. > > > Looks like rowBytes can also be const? > > > > Yes. > > > Alternatively, maybe we could use a struct similar to crrev.com/1396323007? > > +1. I'm happy to use structs for "sizes" and "rowBytes". Using structs now. "sizes" and "widthBytes" are const. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:461: SkASSERT(8 == DCTSIZE); On 2016/01/04 18:29:16, scroggo wrote: > Can this be a compile time assert (i.e. static_assert)? Yes! https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:474: bool unitUV = (1 == dinfo->comp_info[1].h_samp_factor) && On 2016/01/04 18:29:16, scroggo wrote: > Instead of creating this boolean that is just read at the end, what if we > instead said: > > if (!<unitUV>) { > return false; > } > > ... > > return <commonY> Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:483: // Could it be useful to do the same here? On 2016/01/04 18:29:16, scroggo wrote: > If we're going to replace/merge with Chromium, it would make sense to report the > same. Currently, the gpu determines the subsampling type by looking at the Y, U, and V sizes. While chrome has a function that reports an enum, AFAICT it is not used. Other than to confirm that the subsampling ratio is not YUV_UNKNOWN. I don't think we need to provide this unless somebody starts asking for it. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:551: yuv[0] = &rowptrs[0]; // Y rows (8 or 16) On 2016/01/04 18:29:16, scroggo wrote: > nit: Line up these comments? Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:558: rowptrs[i] = (JSAMPROW) SkTAddOffset<void>(pixels[0], i * rowBytes[0]); On 2016/01/04 18:29:16, scroggo wrote: > I think part of the point of SkTAddOffset is that it does the cast for you? > Can't this be > > rowptrs[i] = SkTAddOffset<JSAMPROW>(...); Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:567: int blockIncrementU = 8 * rowBytes[1]; On 2016/01/04 18:29:16, scroggo wrote: > Do all these "8"s refer to DCTSIZE? Yes. Should we use that constant instead? Done. I'm indifferent. I'm not sure if it makes things more or less clear. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:571: // We intentionally round down here. As a final step, we may need to decode one On 2016/01/04 18:29:16, scroggo wrote: > Why do we intentionally round down? > > And why might we need to decode one more partial block? I'll improve this comment. Essentially, if the image height is 21, we will first: First, handle two normal blocks of 8 rows. Second, handle the final five rows in a special case at the end. This case is "special" because we need to provide three fake rows to libjpeg-turbo. I wanted to keep all of the dummyRow and remainingRows checks outside of the main loop. Maybe this isn't worth it? I doubt the performance impact is significant. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:614: return kIncompleteInput; On 2016/01/04 18:29:16, scroggo wrote: > Doesn't kIncompleteInput mean (in getPixels etc) that we do display what we can > and fill the rest? It seems like these should be kInvalidInput until we > implement filling? Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:618: // FIXME: This is ugly. We should find a solution where the input memory is On 2016/01/04 18:29:16, scroggo wrote: > Does making the query call return both the logical and optimal sizes (as in > crrev.com/863053002) help with this problem? Yes I believe so. https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp File tests/JpegTest.cpp (right): https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:521: if (expectedSizes) { On 2016/01/04 18:29:16, scroggo wrote: > Instead of this if/else block, how do you feel about > > REPORTER_ASSERT(reporter, (expectedSizes == nullptr) == !success); > > or something like that? Done. https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:531: REPORTER_ASSERT(reporter, expectedSizes[0].width() == sizes[0].width()); On 2016/01/04 18:29:16, scroggo wrote: > Alternatively, you could do a memcmp? I don't have a strong preference, but that > would be approximately one line (and I think you do this elsewhere). Done. https://codereview.chromium.org/1549473003/diff/60001/tests/JpegTest.cpp#newc... tests/JpegTest.cpp:538: // Calculate rowBytes for the YUV decode On 2016/01/04 18:29:16, scroggo wrote: > nit: It seems like no "calculation" is done. Looks like you just set it to the > width? Acknowledged. Removed this code anyway.
I had a couple of nits that did not get addressed in the last patch set. Otherwise I like the direction. Comments and API 1gtm. > "In addition, should we update DM to test getYUV? Or leave that to a > separate change?" > > I'm working on this. Will probably make it a separate change > unless you feel strongly. Without adding to DM, we've added a feature for drawing, but we haven't added any drawing tests. (I think the tests just test for the correctness of the sizes?) Maybe drawing will reveal that something should change? https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:567: int blockIncrementU = 8 * rowBytes[1]; On 2016/01/12 14:25:27, msarett wrote: > On 2016/01/04 18:29:16, scroggo wrote: > > Do all these "8"s refer to DCTSIZE? > > Yes. > > Should we use that constant instead? > > Done. I'm indifferent. I'm not sure if it makes things more or less clear. I think it's more clear. It is more obvious to me why you would be using DCTSIZE than 8 https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:533: YUVPlanesSizes requiredSizes; Maybe these should be "min" Sizes/WidthBytes, rather than "required"? https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:601: int numIters = dinfo->output_height / numRowsPerBlock; nit: can be const? https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:603: while (iters < numIters) { Can be a for loop?
> I had a couple of nits that did not get addressed in the last patch set. My mistake. Addressing the nits. > "In addition, should we update DM to test getYUV? Or leave that to a > separate change?" > > > I'm working on this. Will probably make it a separate change > > unless you feel strongly. > Without adding to DM, we've added a feature for drawing, but we haven't added > any drawing tests. (I think the tests just test for the correctness of the > sizes?) Maybe drawing will reveal that something should change? I agree that the testing is inadequate without adding drawing tests to DM. I spoke with Brian, and it seems like testing would require a fair bit of work on his end to add an API that we could use. I thought it would be fine to hold off until on testing until we make SkImageGenerator use Codec (so we can hook into the existing test flow). I started work on DM testing and it is here: https://codereview.chromium.org/1581653003/ https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:481: // TODO: Chromium reports the YUV subsampling factors to the client. On 2016/01/04 18:29:16, scroggo wrote: > nit: blank line before comment. Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:513: fDecoderMgr->returnFailure("onGetYUV8Sizes", kInvalidInput); On 2016/01/04 18:29:16, scroggo wrote: > return Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:561: rowptrs[i + 16] =(JSAMPROW) SkTAddOffset<void>(pixels[1], i * rowBytes[1]); On 2016/01/04 18:29:16, scroggo wrote: > nit: space between "=" and the rhs. Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:573: int numIters = dinfo->output_height / numRowsPerBlock; On 2016/01/04 18:29:15, scroggo wrote: > can be const? Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:575: while (iters < numIters) { On 2016/01/04 18:29:16, scroggo wrote: > It looks like iters is only used inside the loop. Can this be a for loop? Done. https://codereview.chromium.org/1549473003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:610: JDIMENSION linesRead = jpeg_read_raw_data(dinfo, yuv, numRowsPerBlock); On 2016/01/04 18:29:15, scroggo wrote: > Should this use |remainingRows| instead of |numRowsPerBlock|? Actually it must use |numRowsPerBlock|. libjpeg/turbo expects us to provide memory for numRowsPerBlock and to ask for numRowsPerBlock. Even though, the extra memory will not be populated with YUV values. https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:533: YUVPlanesSizes requiredSizes; On 2016/01/13 19:34:48, scroggo wrote: > Maybe these should be "min" Sizes/WidthBytes, rather than "required"? minWidthBytes works for me. I don't think minSizes is accurate since we expect the size to match exactly. How about expectedSizes? https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:601: int numIters = dinfo->output_height / numRowsPerBlock; On 2016/01/13 19:34:48, scroggo wrote: > nit: can be const? Done. https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:603: while (iters < numIters) { On 2016/01/13 19:34:48, scroggo wrote: > Can be a for loop? Done.
> I agree that the testing is inadequate without adding > drawing tests to DM. I spoke with Brian, and it seems like > testing would require a fair bit of work on his end to add > an API that we could use. > > I thought it would be fine to hold off until on testing > until we make SkImageGenerator use Codec (so we can hook > into the existing test flow). Ah - using the existing test flow is exactly how I think we should do it! The change to make SkImageGenerator use Codec is at crrev.com/1487683004, right? And that is held up by this, and maybe some code deletions? Does anyone rely on the existing SkImageGenerator (the one that uses SkImageDecoder) implementing YUV? If it's just tests, can we just temporarily disable them until we land this one (with DM testing pulled in)? https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:533: YUVPlanesSizes requiredSizes; On 2016/01/13 20:50:25, msarett wrote: > On 2016/01/13 19:34:48, scroggo wrote: > > Maybe these should be "min" Sizes/WidthBytes, rather than "required"? > > minWidthBytes works for me. > > I don't think minSizes is accurate since we expect the size to match exactly. > How about expectedSizes? sgtm. requiredSizes is also fine. I overgeneralized my complaint for requiredWidthBytes.
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
On 2016/01/13 21:10:46, scroggo wrote: > > I agree that the testing is inadequate without adding > > drawing tests to DM. I spoke with Brian, and it seems like > > testing would require a fair bit of work on his end to add > > an API that we could use. > > > > I thought it would be fine to hold off until on testing > > until we make SkImageGenerator use Codec (so we can hook > > into the existing test flow). > > Ah - using the existing test flow is exactly how I think we should do it! The > change to make SkImageGenerator use Codec is at crrev.com/1487683004, right? And > that is held up by this, and maybe some code deletions? Does anyone rely on the > existing SkImageGenerator (the one that uses SkImageDecoder) implementing YUV? > If it's just tests, can we just temporarily disable them until we land this one > (with DM testing pulled in)? > I've landed SkCodecImageGenerator and used it to add a test to DM. > https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.cpp > File src/codec/SkJpegCodec.cpp (right): > > https://codereview.chromium.org/1549473003/diff/100001/src/codec/SkJpegCodec.... > src/codec/SkJpegCodec.cpp:533: YUVPlanesSizes requiredSizes; > On 2016/01/13 20:50:25, msarett wrote: > > On 2016/01/13 19:34:48, scroggo wrote: > > > Maybe these should be "min" Sizes/WidthBytes, rather than "required"? > > > > minWidthBytes works for me. > > > > I don't think minSizes is accurate since we expect the size to match exactly. > > How about expectedSizes? > > sgtm. requiredSizes is also fine. I overgeneralized my complaint for > requiredWidthBytes.
I've landed SkCodecImageGenerator and used it to add a test to DM. https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... File src/codec/SkCodecImageGenerator.cpp (right): https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... src/codec/SkCodecImageGenerator.cpp:54: // This function is currently a hack to match the implementation This hack is unbelievably ugly. Maybe this CL should become a mega-change that changes the YUV API everywhere? Or maybe I should just follow-up quickly?
https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (left): https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#oldco... dm/DMSrcSink.cpp:544: // TODO: Once we implement GPU paths (e.g. JPEG YUV), we should use a deferred decode to I assume the TODO has changed here? Or maybe it's just uninteresting to test AndroidCodec with YUV, since AndroidCodec just gives us sampling, which YUV does not support? https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:244: // Test to direct raster backend. Also enable CodecImageGenerator tests on gpu. Does this mean to test CodecImageGenerator on the GPU and nowhere else? https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:245: return (flags.type != SinkFlags::kRaster || flags.approach != SinkFlags::kDirect) && Nit: I think it would be easier to follow this with some if statements instead of a single return statement. https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:282: // FIXME: The gpu backend does not draw kGray sources correctly. Is there a bug filed for this? https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:283: if (kGray_8_SkColorType == gen->getInfo().colorType()) { Alternatively, we could skip before we ever get here. push_codec_srcs could check the codec's color type (which we already do for other reasons) and decide to never create this CodecSrc. (If we go that route, we should put an assert here, and reference where we avoid this case.) https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:288: if (!SkDEPRECATED_InstallDiscardablePixelRef(gen, &bitmap)) { I see this is deprecated. Why not call SkImage::NewFromGenerator? Will that (or some other non-deprecated call path) work? https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... File src/codec/SkCodecImageGenerator.cpp (right): https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... src/codec/SkCodecImageGenerator.cpp:54: // This function is currently a hack to match the implementation On 2016/01/15 19:22:03, msarett wrote: > This hack is unbelievably ugly. > > Maybe this CL should become a mega-change that changes the YUV API everywhere? > > Or maybe I should just follow-up quickly? I would leave changing the YUV API to its own CL, just because it's going to have its own complications - you'll need to change the Chromium code that depends on it. https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... src/codec/SkCodecImageGenerator.cpp:75: // Restore the true widths Just to make sure I follow - the old API *requires* getYUV8Planes to be called first with nullptr == planes, so these values will be set already? https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... File src/codec/SkCodecImageGenerator.h (right): https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... src/codec/SkCodecImageGenerator.h:42: int fYWidth; I think we should initialize these to some bad value (e.g. 0?) and assert that they are not 0 when we actually need to use them. Also, it looks like these aren't needed once we change the API of SkImageGenerator? Can you add a comment and/or macro to only include them until we make the change?
https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (left): https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#oldco... dm/DMSrcSink.cpp:544: // TODO: Once we implement GPU paths (e.g. JPEG YUV), we should use a deferred decode to On 2016/01/19 18:37:48, scroggo wrote: > I assume the TODO has changed here? Or maybe it's just uninteresting to test > AndroidCodec with YUV, since AndroidCodec just gives us sampling, which YUV does > not support? Yes YUV does not support sampling or native scaling. I think testing YUV on SkAndroidCodec is uninteresting. https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:244: // Test to direct raster backend. Also enable CodecImageGenerator tests on gpu. On 2016/01/19 18:37:48, scroggo wrote: > Does this mean to test CodecImageGenerator on the GPU and nowhere else? I decided it would be interesting to test to 8888, 565, and gpu. https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:245: return (flags.type != SinkFlags::kRaster || flags.approach != SinkFlags::kDirect) && On 2016/01/19 18:37:48, scroggo wrote: > Nit: I think it would be easier to follow this with some if statements instead > of a single return statement. Done. As side comment, I find the concept of a veto() function <return true if you want to skip> pretty confusing. https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:282: // FIXME: The gpu backend does not draw kGray sources correctly. On 2016/01/19 18:37:48, scroggo wrote: > Is there a bug filed for this? I'll file one and reference it here. https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:283: if (kGray_8_SkColorType == gen->getInfo().colorType()) { On 2016/01/19 18:37:48, scroggo wrote: > Alternatively, we could skip before we ever get here. push_codec_srcs could > check the codec's color type (which we already do for other reasons) and decide > to never create this CodecSrc. (If we go that route, we should put an assert > here, and reference where we avoid this case.) Done. https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:288: if (!SkDEPRECATED_InstallDiscardablePixelRef(gen, &bitmap)) { On 2016/01/19 18:37:48, scroggo wrote: > I see this is deprecated. Why not call SkImage::NewFromGenerator? Will that (or > some other non-deprecated call path) work? Ah yes of course. https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... File src/codec/SkCodecImageGenerator.cpp (right): https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... src/codec/SkCodecImageGenerator.cpp:75: // Restore the true widths On 2016/01/19 18:37:48, scroggo wrote: > Just to make sure I follow - the old API *requires* getYUV8Planes to be called > first with nullptr == planes, so these values will be set already? I can't say that it's *required*. That is how it's used though. I'm not sure how a client could know the correct size without asking for it first. This would fail (when it should succeed) if the client somehow knew the correct size without querying for it first. Or it could even think it succeeds (when it should fail) and write to memory that is the wrong size. I think it's tricky to fix, but it could be done by adding a ton of checks. Given that CodecImageGenerator is only used in test code right now, I would prefer to leave as is until the API can be changed. https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... File src/codec/SkCodecImageGenerator.h (right): https://codereview.chromium.org/1549473003/diff/200001/src/codec/SkCodecImage... src/codec/SkCodecImageGenerator.h:42: int fYWidth; On 2016/01/19 18:37:48, scroggo wrote: > I think we should initialize these to some bad value (e.g. 0?) and assert that > they are not 0 when we actually need to use them. > > Also, it looks like these aren't needed once we change the API of > SkImageGenerator? Can you add a comment and/or macro to only include them until > we make the change? Done.
lgtm https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1549473003/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:245: return (flags.type != SinkFlags::kRaster || flags.approach != SinkFlags::kDirect) && On 2016/01/19 22:35:27, msarett wrote: > On 2016/01/19 18:37:48, scroggo wrote: > > Nit: I think it would be easier to follow this with some if statements instead > > of a single return statement. > > Done. > > As side comment, I find the concept of a veto() function <return true if you > want to skip> pretty confusing. Haha, agreed. It confuses me every time I look at it :P But I do understand the intent - return true to mean "yes, I want to veto running on this Sink". I think it would be clearer to have a different name (not sure what exactly - something that means "shouldIRunOnThisSink") and returns true to run.
Can you combine the set of sizes and set of widthytes into a single struct, or do they need to be separatable? https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.... include/codec/SkCodec.h:283: SkISize VSize; nit: odd that you're not using "f" prefix on fields https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.... include/codec/SkCodec.h:299: size_t YWidthBytes; nit: not sure it helps to repeat the typename for each field. did you consider struct YUVPlanesSizes { SkISize fY; SkISize fU; SkISize fV; } struct YUVPlanesWidthBytes { size_t fY; size_t fU; size_t fV; }; ? https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.... include/codec/SkCodec.h:345: Result getYUV8Planes(const YUVPlanesSizes* sizes, void* planes[3], why const* instead of const& for the two structs?
I combined Sizes and WidthBytes into a single YUVSizeInfo struct. https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.... include/codec/SkCodec.h:283: SkISize VSize; On 2016/01/21 18:36:48, reed1 wrote: > nit: odd that you're not using "f" prefix on fields Fixed. https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.... include/codec/SkCodec.h:299: size_t YWidthBytes; On 2016/01/21 18:36:48, reed1 wrote: > nit: not sure it helps to repeat the typename for each field. did you consider > > struct YUVPlanesSizes { > SkISize fY; > SkISize fU; > SkISize fV; > } > > struct YUVPlanesWidthBytes { > size_t fY; > size_t fU; > size_t fV; > }; > > ? I think you're right. However, since I'm combining into a single struct, I will keep the longer names. https://codereview.chromium.org/1549473003/diff/220001/include/codec/SkCodec.... include/codec/SkCodec.h:345: Result getYUV8Planes(const YUVPlanesSizes* sizes, void* planes[3], On 2016/01/21 18:36:48, reed1 wrote: > why const* instead of const& for the two structs? Changed to const&
https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:280: struct YUVSizeInfo { Just so I understand the difference... Size means logical number of bytes for width/height Bytes means allocation number of bytes for width ? https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:312: if (nullptr == sizeInfo) { interesting: why is one parameter optional and the other required? Seems like if the caller is just interested in knowing if YUV is supported than null for both makes some sense. Or, since the colorspace is just an enum, seems like requiring both is not onerous... https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:342: Result getYUV8Planes(const YUVSizeInfo& sizeInfo, void* planes[3]) { Why do we pass the info back in? Can it (legally) be different from what was returned by the query method?
https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:280: struct YUVSizeInfo { On 2016/01/22 16:01:46, reed1 wrote: > Just so I understand the difference... > > Size means logical number of bytes for width/height > Bytes means allocation number of bytes for width > ? Yes that's correct. https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:312: if (nullptr == sizeInfo) { On 2016/01/22 16:01:46, reed1 wrote: > interesting: why is one parameter optional and the other required? Seems like if > the caller is just interested in knowing if YUV is supported than null for both > makes some sense. Or, since the colorspace is just an enum, seems like requiring > both is not onerous... I'm fine with making both optional or both required. What do you think is best? For more background: Making "colorSpace" optional is the same behavior from the old API. IMO, this makes sense because it seems fairly useless here. We always return kJPEG. However, I've kept it because Brian says the GPU needs this enum to distinguish JPEG from other types of YUV. I'm not sure I see the point of making sizeInfo optional. What kind of client wants to know YUV is supported, but doesn't need the size of the planes? https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:342: Result getYUV8Planes(const YUVSizeInfo& sizeInfo, void* planes[3]) { On 2016/01/22 16:01:46, reed1 wrote: > Why do we pass the info back in? Can it (legally) be different from what was > returned by the query method? The WidthBytes may be larger than the recommended WidthBytes (similar to rowBytes I guess). Having said that, I would be in favor of removing this parameter. This means we require that they use the WidthBytes returned from the query() function. This will always be aligned to 8 bytes. Seems reasonable enough. What do you think?
Is 8-byte alignment (for the rows) ALWAYS the correct answer for all codecs? I assumed that was just JPEG's answer, and that's why we made it an explicit out-parameter... Brian, is there any issue with forcing a given rowbytes for these planes?
On 2016/01/22 16:49:37, reed1 wrote: > Is 8-byte alignment (for the rows) ALWAYS the correct answer for all codecs? I > assumed that was just JPEG's answer, and that's why we made it an explicit > out-parameter... > It's only a requirement of the Jpeg implementation (unless the GPU always needs it aligned?). Although I'm not familiar with the other codecs that produce YUV. Maybe we should go back to using separate structs? That way we can only pass in WidthBytes? > Brian, is there any issue with forcing a given rowbytes for these planes?
If we're going to pass some of the spec for the memory layout (e.g. rowbytes) then I'd prefer we pass it all.
https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:336: * the width of the memory allocated be padded to a This comment seems very JPEG specific. Do we need so much detail here, given the comments on the YUVSizeInfo type itself? Should the comments here just say something like: the sizeinfo needs to match exactly the values returned by the query, except that the widthbytes fields may be larger than the values returned by the query (but not smaller). ?
https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:312: if (nullptr == sizeInfo) { On 2016/01/22 16:14:14, msarett wrote: > On 2016/01/22 16:01:46, reed1 wrote: > > interesting: why is one parameter optional and the other required? Seems like > if > > the caller is just interested in knowing if YUV is supported than null for > both > > makes some sense. Or, since the colorspace is just an enum, seems like > requiring > > both is not onerous... > > I'm fine with making both optional or both required. What do you think is best? > > For more background: > > Making "colorSpace" optional is the same behavior from the old API. IMO, this > makes sense because it seems fairly useless here. We always return kJPEG. > However, I've kept it because Brian says the GPU needs this enum to distinguish > JPEG from other types of YUV. > > I'm not sure I see the point of making sizeInfo optional. What kind of client > wants to know YUV is supported, but doesn't need the size of the planes? Any thoughts? https://codereview.chromium.org/1549473003/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:336: * the width of the memory allocated be padded to a On 2016/01/22 18:40:16, reed1 wrote: > This comment seems very JPEG specific. Do we need so much detail here, given the > comments on the YUVSizeInfo type itself? > > Should the comments here just say something like: > > the sizeinfo needs to match exactly the values returned by the query, except > that the widthbytes fields may be larger than the values returned by the query > (but not smaller). > > ? I think the detail is a bit much. Fixed. So are we ok with keeping this parameter (and giving the client flexibility to choose their own WidthBytes)?
lgtm
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1549473003/#ps260001 (title: "Less comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1549473003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549473003/260001
The CQ bit was unchecked by commit-bot@chromium.org
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 msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1549473003/#ps280001 (title: "Fix return value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1549473003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549473003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1549473003/#ps300001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1549473003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549473003/300001
Message was sent while issue was closed.
Description was changed from ========== Add getYUV8Planes() API to SkCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add getYUV8Planes() API to SkCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/b714fb0199e8727ef2b6cddbee7eba6046f01554 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as https://skia.googlesource.com/skia/+/b714fb0199e8727ef2b6cddbee7eba6046f01554 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
