Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1719533002:
Modify YUV codecs to match Skia's API change (Closed)
|
Created:
4 years, 10 months ago by msarett Modified:
4 years, 8 months ago Reviewers:
robertphillips, reed2, nyquist, f(malita), DaleCurtis, scroggo_chromium, bsalomon, Noel Gordon, noel CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, Daniele Castagna, dshwang, drott+blinkwatch_chromium.org, krit, feature-media-reviews_chromium.org, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionModify YUV codecs to match Skia's API change
This includes a manual roll to:
4984c3c95f18eda44492a2126c9958e447f2cca8
The Skia change is here:
https://codereview.chromium.org/1716523002/
This should also save a few rows of memory
per image.
BUG=
Committed: https://crrev.com/2782f0dd0af04e1bdc226938298d8a6957523f3a
Cr-Commit-Position: refs/heads/master@{#380432}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Assert YUV format in onGetYUV8Planes(), use public test image #
Total comments: 6
Patch Set 3 : ASSERT in base class YUV implementations, Use ArrayBuffer #
Total comments: 14
Patch Set 4 : Response to Patch Set 3 #
Total comments: 18
Patch Set 5 : Response to comments from Patch Set 4 #Patch Set 6 : Rebase #Patch Set 7 : Upload cropped_mandrill.jpg #Patch Set 8 : Add DEPS change #Patch Set 9 : Fix media formatting #Patch Set 10 : Fix blimp #Patch Set 11 : Update DEPS again #Messages
Total messages: 71 (30 generated)
Description was changed from ========== Modify YUV codecs to match Skia's API change BUG= ========== to ========== Modify YUV codecs to match Skia's API change The Skia change is here: https://codereview.chromium.org/1716523002/ BUG= ==========
Description was changed from ========== Modify YUV codecs to match Skia's API change The Skia change is here: https://codereview.chromium.org/1716523002/ BUG= ========== to ========== Modify YUV codecs to match Skia's API change The Skia change is here: https://codereview.chromium.org/1716523002/ BUG= ==========
Description was changed from ========== Modify YUV codecs to match Skia's API change The Skia change is here: https://codereview.chromium.org/1716523002/ BUG= ========== to ========== Modify YUV codecs to match Skia's API change The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + dalecurtis@chromium.org, fmalita@chromium.org, reed@chromium.org, scroggo@chromium.org
https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:931: JSAMPROW yLastRow = *samples; No need to track the last row and perform a memcpy(). The new YUV API guarantees that each planes will be HEIGHT*WIDTH_BYTES in size. https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:686: bpp = 4; I suspect that this can/should be 3. But that is unrelated to this change. https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:944: JSAMPROW dummyRow = *samples; Notice that we use a dummy row when the height of the current block exceeds the actual height - this allows us to only allocate memory for the true height (and not worry about kSizeForMemoryAllocation). https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:236: const char* jpegFileNon8 = "/LayoutTests/fast/images/resources/non8dims.jpg"; // 227x149 Does this need to be a public image? I might need to choose a different one.
https://codereview.chromium.org/1719533002/diff/20001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1719533002/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:251: sizeInfo->fSizes[plane].set(size.width(), size.height()); I think the indentation is wrong here? https://codereview.chromium.org/1719533002/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:265: return false; It seems like we should never reach this case if queryYUV returned false. Maybe we should instead assert that this if condition is not true? https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:236: const char* jpegFileNon8 = "/LayoutTests/fast/images/resources/non8dims.jpg"; // 227x149 On 2016/02/22 20:56:31, msarett wrote: > Does this need to be a public image? I might need to choose a different one. Yes.
https://codereview.chromium.org/1719533002/diff/20001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1719533002/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:251: sizeInfo->fSizes[plane].set(size.width(), size.height()); On 2016/02/22 21:10:31, scroggo_chromium wrote: > I think the indentation is wrong here? Yes it is! Done. https://codereview.chromium.org/1719533002/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:265: return false; On 2016/02/22 21:10:32, scroggo_chromium wrote: > It seems like we should never reach this case if queryYUV returned false. Maybe > we should instead assert that this if condition is not true? I'm not aware of anyone who calls this without calling queryYUV(). I think that should be fine. Done. I'm also modifying DecodingImageGenerator.cpp to follow the same pattern. https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:236: const char* jpegFileNon8 = "/LayoutTests/fast/images/resources/non8dims.jpg"; // 227x149 On 2016/02/22 21:10:32, scroggo_chromium wrote: > On 2016/02/22 20:56:31, msarett wrote: > > Does this need to be a public image? I might need to choose a different one. > > Yes. Done.
How do you plan to land these two CLs? IIUC the Skia change cannot roll until this lands, and this cannot land until the Skia CL rolls :) I see two options: 1) go through the usual API guard/define contortions or 2) (cowboy version) land the Skia CL => Skia rolls will start failing => prepare a manual roll containing this change => land the manual roll => the API change + update are atomic in the Chromium tree
Hmmm cowboy version sounds more fun if that's an acceptable approach :). I'll just want to get comfortable with the steps for a manual roll beforehand, so everything goes smoothly.
platform/graphics LGTM, but I don't understand the decoder bits - deferring to scroggo/dalecurtis. On 2016/02/22 22:53:11, msarett wrote: > Hmmm cowboy version sounds more fun if that's an acceptable approach :). I'm using it sometimes, but only for minor CLs I know I can whip in 5min. This looks significant enough to make me nervous.
On 2016/02/22 23:48:00, f(malita) wrote: > platform/graphics LGTM, but I don't understand the decoder bits - deferring to > scroggo/dalecurtis. > > On 2016/02/22 22:53:11, msarett wrote: > > Hmmm cowboy version sounds more fun if that's an acceptable approach :). > > I'm using it sometimes, but only for minor CLs I know I can whip in 5min. This > looks significant enough to make me nervous. I think you're right. I'll look into staging it appropriately with API guards etc.
msarett@google.com changed reviewers: + noel@google.com
Noel, Can you please take a look at the JpegDecoder changes? Dale, Are you the right reviewer for the media/renderers/skcanvas_video_renderer.cc? Thanks!
No, reed@ is a better reviewer for that.
https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1719533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:686: bpp = 4; On 2016/02/22 20:56:31, msarett wrote: > I suspect that this can/should be 3. But that is unrelated to this change. According to a comment at the call site [1], it should be big enough to hold an RGBA row. This is where we put the output, which is RGBA. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:139: virtual IntSize decodedYUVSize(int component) const { return decodedSize(); } This predates your CL, but it seems like this base version should never be called - it should only be called if we're using JPEGImageDecoder, which overrides it. I would be tempted to implement this as { ASSERT(false); return IntSize(); } https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:140: virtual size_t decodedYUVWidthBytes(int component) const { return decodedSize().width(); } Maybe describe what this means? Also, similar to above, I think this should never be called. So I would implement this as { ASSERT(false); return 0; } https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:110: SkAutoMalloc buffer(rowBytes[0] * ySize.height() + rowBytes[1] * uSize.height() + rowBytes[2] * vSize.height()); Probably do not want to use an SkAutoMalloc outside of Skia? Not sure what the right thing is in Blink; unique_ptr? (Being sure to use delete[] and not delete!)
msarett@google.com changed reviewers: + davidben@chromium.org
David, Are you the right person to take a look at the changes to media/renderers/skcanvas_video_renderer.cc? Thanks! https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:139: virtual IntSize decodedYUVSize(int component) const { return decodedSize(); } On 2016/02/24 13:09:43, scroggo_chromium wrote: > This predates your CL, but it seems like this base version should never be > called - it should only be called if we're using JPEGImageDecoder, which > overrides it. I would be tempted to implement this as > > { > ASSERT(false); > return IntSize(); > } Yes I agree. Done. https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:140: virtual size_t decodedYUVWidthBytes(int component) const { return decodedSize().width(); } On 2016/02/24 13:09:43, scroggo_chromium wrote: > Maybe describe what this means? > > Also, similar to above, I think this should never be called. So I would > implement this as > > { > ASSERT(false); > return 0; > } Done. https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:110: SkAutoMalloc buffer(rowBytes[0] * ySize.height() + rowBytes[1] * uSize.height() + rowBytes[2] * vSize.height()); On 2016/02/24 13:09:44, scroggo_chromium wrote: > Probably do not want to use an SkAutoMalloc outside of Skia? Not sure what the > right thing is in Blink; unique_ptr? (Being sure to use delete[] and not > delete!) Yeah you're right. Looks like the web tools framework has something. Done.
On 2016/02/24 16:36:11, msarett wrote: > David, > > Are you the right person to take a look at the changes to > media/renderers/skcanvas_video_renderer.cc? > Er. Beyond a couple of C++11 CLs, I do not believe I have ever touched even the media directory, much less that file, so probably not. :-P
msarett@google.com changed reviewers: + rileya@google.com - davidben@chromium.org
Ahh ok thanks! Riley, Are you the right person to take a look at the changes to media/renderers/skcanvas_video_renderer.cc?
lgtm
rileya@ doesn't work on chromium anymore. I can stamp the skcanvas changes if a SKIA reviewer is happy with them, cc:dcastagna
msarett@google.com changed reviewers: + bsalomon@google.com - rileya@google.com
"rileya@ doesn't work on chromium anymore. I can stamp the skcanvas changes if a SKIA reviewer is happy with them, cc:dcastagna" Brian, Let me know if you want to take a look at media/renderers/skcanvas_video_renderer.cc or if you know who might be a good reviewer. Otherwise, I think scroggo's review might be the most appropriate in terms of Skia reviewers.
Dale, Would you mind rubber stamping the skcanvas code? scroggo@ is the best skia reviewer. Noel, Could you take a look at the ImageDecoder changes?
On 2016/02/25 14:03:40, msarett wrote: > "rileya@ doesn't work on chromium anymore. I can stamp the skcanvas changes if a > SKIA reviewer is happy with them, cc:dcastagna" > > Brian, > > Let me know if you want to take a look at > media/renderers/skcanvas_video_renderer.cc or if you know who might be a good > reviewer. Otherwise, I think scroggo's review might be the most appropriate in > terms of Skia reviewers. Those changes lgtm
On 2016/02/24 01:03:57, msarett wrote: > Noel, > > Can you please take a look at the JpegDecoder changes? Sure, I was OOO last week btw ...
noel@chromium.org changed reviewers: + noel@chromium.org
https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:83: TRACE_EVENT1("blink", "DecodingImageGenerator::getYUV8Planes", "sizes", static_cast<int>(m_frameIndex)); DecodingImageGenerator::getYUV8Planes -> DecodingImageGenerator::queryYUV8 here? https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:64: bool onGetYUV8Planes(const SkYUVSizeInfo&, void*[3]) override; void*[3] -> void* planes[3] here? https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:137: // Decoders which support YUV decoding must override this to which/that confusion: "Image decoders that support YUV ..." https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:145: // Decoders which support YUV decoding must override this to which/that confusion: "Image decoders that support YUV ..." https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:678: abbrv: what is bpp? https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:683: bpp = 1; We seem to be allocating less memory for the YCbCr samples buffer now. Given the changes to outputRawData(), hard for me to tell if these changes are sound memory-wise. How is this tested? I ask because I'm pretty sure there are no layout tests for JPEG with various YUV orderings (4:4:4, 4:2:2, 4:2:0 etc). https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:237: const char* jpegFileNon8 = "/LayoutTests/fast/images/resources/cropped_mandrill.jpg"; // 439x154 What is the meaning of "Non8" here?
> Sure, I was OOO last week btw ... Thanks for your review! I've responded to comments and also fixed a minor bug in JpegImageDecoder.cpp. https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:83: TRACE_EVENT1("blink", "DecodingImageGenerator::getYUV8Planes", "sizes", static_cast<int>(m_frameIndex)); On 2016/03/01 02:27:48, noel gordon wrote: > DecodingImageGenerator::getYUV8Planes -> DecodingImageGenerator::queryYUV8 here? Yes nice catch! https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:64: bool onGetYUV8Planes(const SkYUVSizeInfo&, void*[3]) override; On 2016/03/01 02:27:49, noel gordon wrote: > void*[3] -> void* planes[3] here? sgtm https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:137: // Decoders which support YUV decoding must override this to On 2016/03/01 02:27:49, noel gordon wrote: > which/that confusion: "Image decoders that support YUV ..." Done. https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:145: // Decoders which support YUV decoding must override this to On 2016/03/01 02:27:49, noel gordon wrote: > which/that confusion: "Image decoders that support YUV ..." Done. https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:678: On 2016/03/01 02:27:49, noel gordon wrote: > abbrv: what is bpp? bytesPer(Pixel/YSample/USample/VSample). I've rewritten this in an attempt to make it more clear. https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:683: bpp = 1; On 2016/03/01 02:27:49, noel gordon wrote: > We seem to be allocating less memory for the YCbCr samples buffer now. Given > the changes to outputRawData(), hard for me to tell if these changes are sound > memory-wise. How is this tested? I ask because I'm pretty sure there are no > layout tests for JPEG with various YUV orderings (4:4:4, 4:2:2, 4:2:0 etc). The jpeg library requires that the memory widths and heights for the Y, U, and V planes are multiples of DCTSIZE (8). That is why we computeYUVWidthBytes() instead of width, to make sure all of the rows are padded appropriately. Luckily, libjpeg/libjpeg-turbo wants a 2D array (JSAMPLE**) for its input planes. So, for the unnecessary rows at the bottom, we can pass the same pointer to a single extra memory buffer. Cases that I think are interesting to test (due to my implementation changes) are dimensions that are multiples of 8 and dimensions that are not multiples of 8. I have added a test to JpegImageDecoderTest to make sure we cover these two cases. I'm not extremely familiar with LayoutTests/visual correctness tests for jpeg decodes in chromium, so I'll defer to your judgment. But looking over the set of images in src/third_party/WebKit/LayoutTests/fast/images/resources/, I feel comfortable that this will be well tested. https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:237: const char* jpegFileNon8 = "/LayoutTests/fast/images/resources/cropped_mandrill.jpg"; // 439x154 On 2016/03/01 02:27:49, noel gordon wrote: > What is the meaning of "Non8" here? Just that the dimensions are not multiples of 8. I'll try to make this more clear.
On 2016/03/01 18:00:38, msarett wrote: > https://codereview.chromium.org/1719533002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:683: > bpp = 1; > On 2016/03/01 02:27:49, noel gordon wrote: > > We seem to be allocating less memory for the YCbCr samples buffer now. Given > > the changes to outputRawData(), hard for me to tell if these changes are sound > > memory-wise. How is this tested? I ask because I'm pretty sure there are no > > layout tests for JPEG with various YUV orderings (4:4:4, 4:2:2, 4:2:0 etc). > > Cases that I think are interesting to test (due to my implementation changes) > are dimensions that are multiples of 8 and dimensions that are not multiples of > 8. I have added a test to JpegImageDecoderTest to make sure we cover these two > cases. OK good, thank you. > I'm not extremely familiar with LayoutTests/visual correctness tests for jpeg > decodes in chromium, so I'll defer to your judgment. But looking over the set > of images in src/third_party/WebKit/LayoutTests/fast/images/resources/, > I feel comfortable that this will be well tested. I think YUV JPEG decoding is enabled by: * setting the --force-gpu-rasterization flag - I'm not sure if we have a virtual layout test suite for this flag. * or by setting a page <meta> tag that Android devices read and use to automatically turn on the --force-gpu-rasterization flag. - the magic <meta> tag is not present in any image layout test page, last I looked. I suppose you could put an exit(1) in decodeToYUV() to cause a crash, and then run the layout tests locally to find out if YUV decoding is active during layout tests.
Patch is looking good, some minor nits. https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:227: override { Style nit: chromium line limits /meh causing an override keyword placement issue. Maybe shut (and roll) our eyes and write it as: bool onQueryYUV8(SkYUVSizeInfo* sizeInfo, SkYUVColorSpace* color_space) const override { ... https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:259: { Style nit: chromium & line limits & weird readability line breaks. Best I can tell, this style contortion has to be handled like so: bool onGetYUV8Planes(const SkYUVSizeInfo& sizeInfo, void* planes[3]) override { ... https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:262: Optional: store or ref frame->format() into a local variable called |format| and then write the DCHECK as: DCHECK(media::IsYuvPlanar(format) && format != PIXEL_FORMAT_YV12A); to improve readability, if you prefer that. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:241: RELEASE_ASSERT(sizeUpdated); We lost an ASSERT here. Something like this ASSERT(decoder->isDecodedSizeAvailable() && decoder->canDecodeToYUV()); would document the contract? https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:84: bool decodeToYUV(size_t index, const SkISize componentSizes[3], void* planes[3], const size_t rowBytes[3]); Not really to do with your CL, but would it make sense to move to planes to the end of the argument list (since it is really the "out" parameter)? bool decodeToYUV(size_t index, const SkISize componentSizes[3], const size_t rowBytes[3], void* planes[3]); https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:686: // as large as the U and V plane widths. Thanks for this. Where Blink code is very tricky, comments are appropriate. Nit: prefer the early return. In this case you could do this first if (m_info.out_color_space != JCS_YCbCr) return (*m_info.mem->alloc_sarray)(reinterpret_cast_ptr ...); then go on to comment and code the JCS_YCbCr special case. Helps avoid "if-else-y code" disappearing off to the right of the page. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (left): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:47: bool setSize(unsigned width, unsigned height) override; Nit: move this setSize() declaration line; put it just after the decodedSize() declaration (so all the YUV-related decls are grouped together). https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:116: OwnPtr<ImagePlanes> imagePlanes = adoptPtr(new ImagePlanes(planes, rowBytes)); Nit: space before this line. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:237: const char* jpegFileImageSizeNotMultipleOf8 = "/LayoutTests/fast/images/resources/cropped_mandrill.jpg"; // 439x154 Thanks for the name fix: works for me.
Patchset #5 (id:100001) has been deleted
> I think YUV JPEG decoding is enabled by: > > * setting the --force-gpu-rasterization flag > - I'm not sure if we have a virtual layout test suite for this flag. > > * or by setting a page <meta> tag that Android devices read and use to > automatically turn on the --force-gpu-rasterization flag. > - the magic <meta> tag is not present in any image layout test page, > last I looked. > > I suppose you could put an exit(1) in decodeToYUV() to cause a crash, and then > run the layout tests locally to find out if YUV decoding is active during layout > tests. Thanks for this explanation. You're right, by default, layout tests doesn't really run any of this stuff. https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:227: override { On 2016/03/03 01:25:28, noel gordon wrote: > Style nit: chromium line limits /meh causing an override keyword placement > issue. Maybe shut (and roll) our eyes and write it as: > > bool onQueryYUV8(SkYUVSizeInfo* sizeInfo, > SkYUVColorSpace* color_space) const override { > ... Done. https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:259: { On 2016/03/03 01:25:28, noel gordon wrote: > Style nit: chromium & line limits & weird readability line breaks. Best I can > tell, this style contortion has to be handled like so: > > bool onGetYUV8Planes(const SkYUVSizeInfo& sizeInfo, > void* planes[3]) override { > ... Done. https://codereview.chromium.org/1719533002/diff/80001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:262: On 2016/03/03 01:25:28, noel gordon wrote: > Optional: store or ref frame->format() into a local variable called |format| and > then write the DCHECK as: > > DCHECK(media::IsYuvPlanar(format) && format != PIXEL_FORMAT_YV12A); > > to improve readability, if you prefer that. Done. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:241: RELEASE_ASSERT(sizeUpdated); On 2016/03/03 01:25:28, noel gordon wrote: > We lost an ASSERT here. Something like this > > ASSERT(decoder->isDecodedSizeAvailable() && decoder->canDecodeToYUV()); > > would document the contract? > Added the assert for canDecodeToYUV(). That gives us back the old coverage. I don't think it's safe to assert isDecodedSizeAvailable() since setSize() doesn't get called until the decode() function. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:84: bool decodeToYUV(size_t index, const SkISize componentSizes[3], void* planes[3], const size_t rowBytes[3]); On 2016/03/03 01:25:28, noel gordon wrote: > Not really to do with your CL, but would it make sense to move to planes to the > end of the argument list (since it is really the "out" parameter)? > > bool decodeToYUV(size_t index, const SkISize componentSizes[3], const size_t > rowBytes[3], void* planes[3]); I don't disagree with you... But on the other hand, (Size, Memory, RowBytes) is a common pattern in Skia (though I realize this isn't Skia). If you don't mind, I'll leave as is in this CL. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:686: // as large as the U and V plane widths. On 2016/03/03 01:25:28, noel gordon wrote: > Thanks for this. Where Blink code is very tricky, comments are appropriate. > > Nit: prefer the early return. In this case you could do this first > > if (m_info.out_color_space != JCS_YCbCr) > return (*m_info.mem->alloc_sarray)(reinterpret_cast_ptr ...); > > then go on to comment and code the JCS_YCbCr special case. Helps avoid > "if-else-y code" disappearing off to the right of the page. Done. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (left): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:47: bool setSize(unsigned width, unsigned height) override; On 2016/03/03 01:25:28, noel gordon wrote: > Nit: move this setSize() declaration line; put it just after the decodedSize() > declaration (so all the YUV-related decls are grouped together). Done. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:116: OwnPtr<ImagePlanes> imagePlanes = adoptPtr(new ImagePlanes(planes, rowBytes)); On 2016/03/03 01:25:28, noel gordon wrote: > Nit: space before this line. Done. https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:237: const char* jpegFileImageSizeNotMultipleOf8 = "/LayoutTests/fast/images/resources/cropped_mandrill.jpg"; // 439x154 On 2016/03/03 01:25:28, noel gordon wrote: > Thanks for the name fix: works for me. Acknowledged.
LGTM On 2016/03/04 19:49:09, msarett wrote: > > I think YUV JPEG decoding is enabled by: > > > > * setting the --force-gpu-rasterization flag > > - I'm not sure if we have a virtual layout test suite for this flag. > > > > * or by setting a page <meta> tag that Android devices read and use to > > automatically turn on the --force-gpu-rasterization flag. > > - the magic <meta> tag is not present in any image layout test page, > > last I looked. > > > > I suppose you could put an exit(1) in decodeToYUV() to cause a crash, and then > > run the layout tests locally to find out if YUV decoding is active during > > layout tests. > > Thanks for this explanation. You're right, by default, layout tests doesn't > really run any of this stuff. Yes, and layout tests also give us MSan, TSan, and ClusterFuzz coverage. Patch https://codereview.chromium.org/561363002 planned to add tests. Matt, please file a bug, and chat to Alexis off-line to see if we can recover that patch? (The patch contains binary files and I can't patch them into my client from the CL issue patch diff). > https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): > > https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:241: > RELEASE_ASSERT(sizeUpdated); > On 2016/03/03 01:25:28, noel gordon wrote: > > We lost an ASSERT here. Something like this > > > > ASSERT(decoder->isDecodedSizeAvailable() && decoder->canDecodeToYUV()); > > > > would document the contract? > > Added the assert for canDecodeToYUV(). That gives us back the old coverage. > > I don't think it's safe to assert isDecodedSizeAvailable() since setSize() > doesn't get called until the decode() function. Seems fine. > https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right): > > https://codereview.chromium.org/1719533002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:84: bool > decodeToYUV(size_t index, const SkISize componentSizes[3], void* planes[3], > const size_t rowBytes[3]); > On 2016/03/03 01:25:28, noel gordon wrote: > > Not really to do with your CL, but would it make sense to move to planes to > the > > end of the argument list (since it is really the "out" parameter)? > > > > bool decodeToYUV(size_t index, const SkISize componentSizes[3], const size_t > > rowBytes[3], void* planes[3]); > > I don't disagree with you... > > But on the other hand, (Size, Memory, RowBytes) is a common pattern in Skia > (though I realize this isn't Skia). > > If you don't mind, I'll leave as is in this CL. No problem, let's leave it as is.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719533002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719533002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Dale, would you mind rubber stamping media/renderers/skcanvas_video_renderer.cc? > > Thanks for this explanation. You're right, by default, layout tests doesn't > > really run any of this stuff. > > Yes, and layout tests also give us MSan, TSan, and ClusterFuzz coverage. Patch > https://codereview.chromium.org/561363002 planned to add tests. Matt, please > file a bug, and chat to Alexis off-line to see if we can recover that patch? > (The patch contains binary files and I can't patch them into my client from the > CL issue patch diff). I'll be sure to follow up here.
media/ rs lgtm based on skia@ approval.
Description was changed from ========== Modify YUV codecs to match Skia's API change The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ========== to ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 095d31c8a0eeb5d491febf064bc3c8a44e22b94f The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ==========
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, scroggo@chromium.org, bsalomon@google.com, noel@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1719533002/#ps180001 (title: "Add DEPS change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719533002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719533002/180001
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, scroggo@chromium.org, fmalita@chromium.org, noel@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1719533002/#ps200001 (title: "Fix media formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719533002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719533002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719533002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719533002/240001
msarett@google.com changed reviewers: + nyquist@chromium.org
nyquist@ Could you please take a look at the blimp changes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
blimp lgtm
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, scroggo@chromium.org, fmalita@chromium.org, noel@chromium.org, dalecurtis@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1719533002/#ps260001 (title: "Update DEPS again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719533002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719533002/260001
Description was changed from ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 095d31c8a0eeb5d491febf064bc3c8a44e22b94f The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ========== to ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 4984c3c95f18eda44492a2126c9958e447f2cca8 The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719533002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719533002/260001
Message was sent while issue was closed.
Description was changed from ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 4984c3c95f18eda44492a2126c9958e447f2cca8 The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ========== to ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 4984c3c95f18eda44492a2126c9958e447f2cca8 The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 4984c3c95f18eda44492a2126c9958e447f2cca8 The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= ========== to ========== Modify YUV codecs to match Skia's API change This includes a manual roll to: 4984c3c95f18eda44492a2126c9958e447f2cca8 The Skia change is here: https://codereview.chromium.org/1716523002/ This should also save a few rows of memory per image. BUG= Committed: https://crrev.com/2782f0dd0af04e1bdc226938298d8a6957523f3a Cr-Commit-Position: refs/heads/master@{#380432} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2782f0dd0af04e1bdc226938298d8a6957523f3a Cr-Commit-Position: refs/heads/master@{#380432} |
