Issue 1530933003: Use possible read_partial_scanlines() API in SkJpegCodec

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+202 lines, -161 lines)			Patch
M	dm/DM.cpp	View	1	2 chunks	+3 lines, -3 lines	0 comments	Download
M	dm/DMSrcSink.h	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	dm/DMSrcSink.cpp	View	1	1 chunk	+25 lines, -40 lines	0 comments	Download
M	gyp/codec.gyp	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	gyp/libjpeg-turbo.gyp	View		5 chunks	+114 lines, -113 lines	0 comments	Download
M	src/codec/SkCodec.cpp	View	1	1 chunk	+8 lines, -3 lines	0 comments	Download
M	src/codec/SkJpegCodec.h	View	1	1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/codec/SkJpegCodec.cpp	View	1	2 chunks	+46 lines, -1 line	0 comments	Download

Depends on Patchset:

Issue 1188223002 Patch 60001

Messages

Total messages: 7 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

msarett

I'm getting pretty close on the new API for libjpeg-turbo. Just wanted to get your ...

5 years ago (2015-12-16 16:10:09 UTC) #3

scroggo

On 2015/12/16 16:10:09, msarett wrote: > I'm getting pretty close on the new API for ...

5 years ago (2015-12-16 18:17:01 UTC) #4

On 2015/12/16 16:10:09, msarett wrote:
> I'm getting pretty close on the new API for libjpeg-turbo.
> 
> Just wanted to get your thoughts on how the API interacts
> with SkJpegCodec before I actually try to upstream it.  I
> don't want to upstream something that we don't want to use.
> 
> Please only review SkJpegCodec.  I've been hacking the test
> code to test what I want, but I don't think that's relevant.

I ignored everything but SkJpegCodec, as requested. This seems like a reasonable
way to do it (modulo comments, which are for minor details). Is there anything
you were worried might be a problem?

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:390: Options subsetOptions = options;
I almost suggested "partialOptions", but it is actually used even when we are
not doing partial scanlines.

Maybe the parameter should be renamed to originalOptions? (This one could also
be named actualOptions.)

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:395: // libjpeg-turbo may need to align startX to a
multiple of the IDCT
nit: blank line before comments

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:400: jpeg_set_partial_scanline(fDecoderMgr->dinfo(),
&startX, &width);
Should we have a fallback if jpeg_set_partial_scanline is not available?

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:406: JCS_CMYK ==
fDecoderMgr->dinfo()->out_color_space) {
I don't understand why we need to check for this here. It only needs you to call
initializeSwizzler, which should be covered by the if statement below?

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:409: SkASSERT(startX + width >= (uint32_t)
options.fSubset->right());
nit: blank line between asserts and code

https://codereview.chromium.org/1530933003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:410: subsetRect.fLeft = options.fSubset->x() - startX;
IIUC, this means that the subset is now the subset of the partial scanline?
Maybe worth a comment?

msarett

5 years ago (2015-12-16 18:23:47 UTC) #5

"I ignored everything but SkJpegCodec, as requested. This seems like a
reasonable
way to do it (modulo comments, which are for minor details). Is there anything
you were worried might be a problem?"

I thought so too.  I have a lot of testing and bug fixing
to do, but I don't think there's a problem.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages