Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1997703003:
Make SkPngCodec decode progressively. (Closed)
|
Created:
4 years, 7 months ago by scroggo_chromium Modified:
4 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@foil Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionMake SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
- Set fDstInfo and fOptions in getPixels(). This may not be necessary
for all implementations, but it simplifies things for SkPngCodec.
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly so).
Some raw performance differences can be found at https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD-pMgO-FXldlNF9ejo4o/
Design doc can be found at https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_Cu8DF57KNtUmgLM/
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1997703003
Committed: https://skia.googlesource.com/skia/+/8e6c7ada5a4c5a950cded765d14d3e6906acdc79
Patch Set 1 #Patch Set 2 : Remove some accidentally added extra lines #Patch Set 3 : Fixes for ICO, Incomplete, comments #
Total comments: 41
Patch Set 4 : Respond to comments #
Total comments: 4
Patch Set 5 : Respond to comments #Patch Set 6 : Remove callback. Use SkSampler to determine which rows to skip. #Patch Set 7 : Switch statement for setjmp #
Total comments: 6
Patch Set 8 : Line length and fix comment #
Total comments: 3
Patch Set 9 : Workaround for BMP in ICO #Patch Set 10 : Fixes for ICO/SampledCodec #
Total comments: 4
Patch Set 11 : Add a fixme #Patch Set 12 : Rebase #Patch Set 13 : Fix conversion error #Patch Set 14 : Use the correct Options object #
Total comments: 4
Patch Set 15 : Disable PNG for older versions of libpng #Patch Set 16 : rebase #Patch Set 17 : Disable ICO when SK_HAS_PNG_LIBRARY gets undefined #Patch Set 18 : Don't compare color space #Patch Set 19 : Pull in crrev.com/2026873002 #Patch Set 20 : Pull in crrev.com/2021393002 #Patch Set 21 : Work around older versions of libpng with SkStream::move #Patch Set 22 : Fix a comment #Patch Set 23 : Add explanatory comment #
Total comments: 1
Patch Set 24 : Add missing override #Patch Set 25 : rebase #Patch Set 26 : Rebase #Patch Set 27 : Rebase #Patch Set 28 : Rename to match test file convention #Patch Set 29 : Rebase #Patch Set 30 : Stop pretending to support older versions of libpng #Patch Set 31 : Rebase #Patch Set 32 : Make some methods private #Patch Set 33 : Consolidate setup for width and xformAlphaType #Patch Set 34 : Simplify applyXformRow #
Total comments: 14
Patch Set 35 : Fix comments, fix CodecPartialTest #Patch Set 36 : Fix a bug interleaving getPixels with incrementalDecode #Patch Set 37 : rebase #
Total comments: 2
Patch Set 38 : Potential rebase (different machine) #Patch Set 39 : Add a comment in response to comments #Patch Set 40 : Simplify applyXformRow #Patch Set 41 : Workaround bug #
Total comments: 5
Patch Set 42 : Make PNG test !incomplete, too #
Total comments: 2
Patch Set 43 : Small cleanups #Patch Set 44 : Support Google3... #Patch Set 45 : Rebase #Messages
Total messages: 100 (45 generated)
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new scanline decoding API for SkCodec, since the existing one
is not easily compatible with using png_process_data. The new API uses
callbacks to determine where and whether to decode a row into the output.
The plan is to transition the other implementations from the old API to
the new one, then delete the old one (startScanlineDecode and get/skip-
Scanlines).
In the new API, the client specifies which rows to decode, and a
callback for telling the SkCodec where to write the result (or NULL to
skip).
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fit kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
BUG=skia:4211
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new scanline decoding API for SkCodec, since the existing one
is not easily compatible with using png_process_data. The new API uses
callbacks to determine where and whether to decode a row into the output.
The plan is to transition the other implementations from the old API to
the new one, then delete the old one (startScanlineDecode and get/skip-
Scanlines).
In the new API, the client specifies which rows to decode, and a
callback for telling the SkCodec where to write the result (or NULL to
skip).
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fit kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
BUG=skia:4211
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new scanline decoding API for SkCodec, since the existing one
is not easily compatible with using png_process_data. The new API uses
callbacks to determine where and whether to decode a row into the output.
The plan is to transition the other implementations from the old API to
the new one, then delete the old one (startScanlineDecode and get/skip-
Scanlines).
In the new API, the client specifies which rows to decode, and a
callback for telling the SkCodec where to write the result (or NULL to
skip).
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fit kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
BUG=skia:4211
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new scanline decoding API for SkCodec, since the existing one
is not easily compatible with using png_process_data. The new API uses
callbacks to determine where and whether to decode a row into the output.
The plan is to transition the other implementations from the old API to
the new one, then delete the old one (startScanlineDecode and get/skip-
Scanlines).
In the new API, the client specifies which rows to decode, and a
callback for telling the SkCodec where to write the result (or NULL to
skip).
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fit kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
scroggo@chromium.org changed reviewers: + msarett@google.com, reed@google.com
https://codereview.chromium.org/1997703003/diff/40001/src/android/SkBitmapReg... File src/android/SkBitmapRegionDecoder.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/android/SkBitmapReg... src/android/SkBitmapRegionDecoder.cpp:45: // supports this scanline ordering. This change is redundant once I rebase and SkBitmapRegionCanvas is gone.
https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:405: * lines decoded. This includes all lines decoded from the image, whether nit: "whether" -> "whether or not" ? https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:776: friend class DM::CodecSrc; // for fillIncompleteImage Is this what our clients will do? If not, can we fill a different way in DM? I'm guessing it's tricky because the "fill value" isn't public? Is there anything else make this difficult for the client (possibly sampling/subsetting)? Maybe we should have a public API to fill(), where the client optionally provides the fill value. I think this could be a follow-up though. UPDATE: The scanline ordering also makes filling hard. I do think we should add a public API that offers to fill for the client... https://codereview.chromium.org/1997703003/diff/40001/src/android/SkBitmapReg... File src/android/SkBitmapRegionDecoder.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/android/SkBitmapReg... src/android/SkBitmapRegionDecoder.cpp:45: // supports this scanline ordering. On 2016/05/19 20:58:38, scroggo_chromium wrote: > This change is redundant once I rebase and SkBitmapRegionCanvas is gone. Acknowledged. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:235: // FIXME: Share more code with getPixels/startScanlineDecode? I think this would be a good chance to share the code that should be common with getPixels(). Though I guess a lot of it is slightly different... IMHO, I'm not worried about sharing with startScanlineDecode(), since we'll be deleting that anyway. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:52: * SkCodec (pointed to by *codecPtr) which will own them, as well as the Codec refs but doesn't own the chunkReader? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:352: std::move(colorSpace)) Why std::move? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:383: return static_cast<SkPngNormalDecoder*>(png_get_progressive_ptr(png_ptr)); I think I've figured it out, but maybe comment on what does png_get_progressive_ptr does? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:392: // FIXME: Make it possible to continue decoding all rows? What does this mean? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:401: if (rowsDecoded) { Should we set rowDecoded even if it's a success? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:453: png_error(this->png_ptr(), "Fake error to stop decoding scanlines."); Can we pass more info to this call so, when we catch it in process data, we know that we are the ones who called it? Will that change how we handle it? I'm happy for this to be a follow up. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:541: if (rowsDecoded) { Same question as above... Should we still set rowsDecoded on a success? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:542: *rowsDecoded = fLinesDecoded; This is what we want for filling... But is it weird for a client to think that we have decoded most/all of the rows when there are more interlaced passes to come. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.h File src/codec/SkPngCodec.h (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.h#... src/codec/SkPngCodec.h:50: void processData(); nit: Comments to explain what this does? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... src/codec/SkSampledCodec.cpp:110: SkIRect incrementalSubset = SkIRect::MakeXYWH(scaledSubsetX, scaledSubsetY, Here's a case where the new API makes more sense :) https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... src/codec/SkSampledCodec.cpp:128: this->codec()->fillIncompleteImage(scaledInfo, pixels, rowBytes, options.fZeroInitialized, nit: 100 chars https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... src/codec/SkSampledCodec.cpp:264: // FIXME: Should this be info or nativeInfo? Does it make a difference? I think it's irrelevant, though the fill() code has become confusing (I might try to refactor/simplify after this lands). Certainly the height is irrelevant, that will be determined by info.height() and lastRowInOutput. As far as width, we want to fill the "actual width of the memory". nativeInfo.width() is not equal to the "actual width" if we are sampling... But SkSwizzler corrects the width if we are sampling anyway.
At a high level, I like how this looks... https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:776: friend class DM::CodecSrc; // for fillIncompleteImage On 2016/05/20 15:04:40, msarett wrote: > Is this what our clients will do? If not, can we fill a different way in DM? > > I'm guessing it's tricky because the "fill value" isn't public? Is there > anything else make this difficult for the client (possibly sampling/subsetting)? > > Maybe we should have a public API to fill(), where the client optionally > provides the fill value. I think this could be a follow-up though. > > UPDATE: The scanline ordering also makes filling hard. I do think we should > add a public API that offers to fill for the client... Might be changing my mind on this again. If we are the client that will handle filling, I think we can get away with it not being public.
https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:405: * lines decoded. This includes all lines decoded from the image, whether On 2016/05/20 15:04:40, msarett wrote: > nit: "whether" -> "whether or not" ? Done. https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:776: friend class DM::CodecSrc; // for fillIncompleteImage On 2016/05/20 15:06:59, msarett wrote: > On 2016/05/20 15:04:40, msarett wrote: > > Is this what our clients will do? If not, can we fill a different way in DM? > > > > I'm guessing it's tricky because the "fill value" isn't public? Is there > > anything else make this difficult for the client (possibly > sampling/subsetting)? > > > > Maybe we should have a public API to fill(), where the client optionally > > provides the fill value. I think this could be a follow-up though. > > > > UPDATE: The scanline ordering also makes filling hard. I do think we should > > add a public API that offers to fill for the client... > > Might be changing my mind on this again. If we are the client that will handle > filling, I think we can get away with it not being public. If Chrome continues to behave the way it currently does, it will fill prior to doing any decoding. If they decide to fill afterwards (without using SkAndroidCodec), we'll need to make this public, but I figured we could decide that once it becomes an issue. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:235: // FIXME: Share more code with getPixels/startScanlineDecode? On 2016/05/20 15:04:40, msarett wrote: > I think this would be a good chance to share the code that should be common with > getPixels(). Though I guess a lot of it is slightly different... I moved the checks for index8, colortables, etc into a macro. That's the part I really meant, that is shared across all methods. The rest is slightly different or trivial. > > IMHO, I'm not worried about sharing with startScanlineDecode(), since we'll be > deleting that anyway. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:52: * SkCodec (pointed to by *codecPtr) which will own them, as well as the On 2016/05/20 15:04:40, msarett wrote: > Codec refs but doesn't own the chunkReader? I'm not sure what you're asking. Do you want a clearer comment here? Technically, yes, the Codec will ref the chunkReader, and may not be the only owner. How about this: "... a new SkCodec (pointed to by *codecPtr) which will own/ref them, ..." https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:352: std::move(colorSpace)) On 2016/05/20 15:04:40, msarett wrote: > Why std::move? My understanding of sk_sp is that if I do not std::move it, there will be unnecessary ref count churn: The constructor (INHERITED) takes an sk_sp<SkColorSpace>. If I pass it colorSpace, it will call the copy constructor which will call ref. At the end of this constructor, colorSpace will go out of scope and call unref(). On the other hand, if I call std::move, INHERITED will call the move constructor, which will not call ref/unref. Is my understanding correct? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:383: return static_cast<SkPngNormalDecoder*>(png_get_progressive_ptr(png_ptr)); On 2016/05/20 15:04:41, msarett wrote: > I think I've figured it out, but maybe comment on what does > png_get_progressive_ptr does? I'm not sure it's necessary to document the methods we call in other libraries (libpng has its own documentation for this method, FWIW). As a compromise, I've added a comment the first time I use it. You also made me question the fact that read_header set the progressive_ptr, and AutoCleanPng uses it. I've moved that instance into AutoCleanPng. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:392: // FIXME: Make it possible to continue decoding all rows? On 2016/05/20 15:04:40, msarett wrote: > What does this mean? This comment is probably out of place here. Removed. (FWIW, it means that I think it would be nice to have a way for the client to say "I want all rows, incrementally". I imagined they might do that by calling getPixels(), but we don't support that yet (a little more thoughts at [1]). If we were to add that support, and continue calling through decodeAllRows, we would need to not reset this here.) [1] https://docs.google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_Cu8DF57KNtUmg... https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:401: if (rowsDecoded) { On 2016/05/20 15:04:41, msarett wrote: > Should we set rowDecoded even if it's a success? I don't think so. On success the client need not read this variable. FWIW, onGetPixels already behaves this way. My comment in the header does not specify this, though, so I have updated it. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:453: png_error(this->png_ptr(), "Fake error to stop decoding scanlines."); On 2016/05/20 15:04:41, msarett wrote: > Can we pass more info to this call so, when we catch it in process data, we know > that we are the ones who called it? Not to png_error, but we could call longjmp directly, which would allow us to pass a special value. (png_error calls a method we specify. We make it call sk_error_fn, which arbitrarily uses the value 1. We could specify 1 there and something else here.) > > Will that change how we handle it? I'm not sure... > > I'm happy for this to be a follow up. sgtm. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:541: if (rowsDecoded) { On 2016/05/20 15:04:40, msarett wrote: > Same question as above... Should we still set rowsDecoded on a success? Same answer as above :) I don't think we need to, since there's no reason for the client to read it on kSuccess. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:542: *rowsDecoded = fLinesDecoded; On 2016/05/20 15:04:40, msarett wrote: > This is what we want for filling... But is it weird for a client to think that > we have decoded most/all of the rows when there are more interlaced passes to > come. Perhaps weird, but I don't know why else the client would want to know how many rows were decoded. I've updated the comment in the header to discuss this behavior. Perhaps I should go further and change the name to rowsInitialized? https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.h File src/codec/SkPngCodec.h (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.h#... src/codec/SkPngCodec.h:50: void processData(); On 2016/05/20 15:04:41, msarett wrote: > nit: Comments to explain what this does? Done. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... src/codec/SkSampledCodec.cpp:110: SkIRect incrementalSubset = SkIRect::MakeXYWH(scaledSubsetX, scaledSubsetY, On 2016/05/20 15:04:41, msarett wrote: > Here's a case where the new API makes more sense :) Agreed. I felt good about the new API when some of my call sites became much simpler. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... src/codec/SkSampledCodec.cpp:128: this->codec()->fillIncompleteImage(scaledInfo, pixels, rowBytes, options.fZeroInitialized, On 2016/05/20 15:04:41, msarett wrote: > nit: 100 chars Done. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkSampledCode... src/codec/SkSampledCodec.cpp:264: // FIXME: Should this be info or nativeInfo? Does it make a difference? On 2016/05/20 15:04:41, msarett wrote: > I think it's irrelevant, though the fill() code has become confusing (I might > try to refactor/simplify after this lands). Certainly the height is irrelevant, > that will be determined by info.height() and lastRowInOutput. > > As far as width, we want to fill the "actual width of the memory". > nativeInfo.width() is not equal to the "actual width" if we are sampling... But > SkSwizzler corrects the width if we are sampling anyway. That was my interpretation as well. > (I might try to refactor/simplify after this lands). sgtm
lgtm I'm comfortable with how this changes our end point within the input stream. https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:776: friend class DM::CodecSrc; // for fillIncompleteImage On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:06:59, msarett wrote: > > On 2016/05/20 15:04:40, msarett wrote: > > > Is this what our clients will do? If not, can we fill a different way in > DM? > > > > > > I'm guessing it's tricky because the "fill value" isn't public? Is there > > > anything else make this difficult for the client (possibly > > sampling/subsetting)? > > > > > > Maybe we should have a public API to fill(), where the client optionally > > > provides the fill value. I think this could be a follow-up though. > > > > > > UPDATE: The scanline ordering also makes filling hard. I do think we > should > > > add a public API that offers to fill for the client... > > > > Might be changing my mind on this again. If we are the client that will > handle > > filling, I think we can get away with it not being public. > > If Chrome continues to behave the way it currently does, it will fill prior to > doing any decoding. If they decide to fill afterwards (without using > SkAndroidCodec), we'll need to make this public, but I figured we could decide > that once it becomes an issue. sgtm https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:52: * SkCodec (pointed to by *codecPtr) which will own them, as well as the On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:40, msarett wrote: > > Codec refs but doesn't own the chunkReader? > > I'm not sure what you're asking. Do you want a clearer comment here? > > Technically, yes, the Codec will ref the chunkReader, and may not be the only > owner. How about this: > > "... a new SkCodec (pointed to by *codecPtr) which will own/ref them, ..." Yes thanks. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:352: std::move(colorSpace)) On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:40, msarett wrote: > > Why std::move? > > My understanding of sk_sp is that if I do not std::move it, there will be > unnecessary ref count churn: > > The constructor (INHERITED) takes an sk_sp<SkColorSpace>. If I pass it > colorSpace, it will call the copy constructor which will call ref. At the end of > this constructor, colorSpace will go out of scope and call unref(). > > On the other hand, if I call std::move, INHERITED will call the move > constructor, which will not call ref/unref. > > Is my understanding correct? That makes sense to me - thanks for the explanation. I should be more thoughtful about using std::move also... https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:383: return static_cast<SkPngNormalDecoder*>(png_get_progressive_ptr(png_ptr)); On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:41, msarett wrote: > > I think I've figured it out, but maybe comment on what does > > png_get_progressive_ptr does? > > I'm not sure it's necessary to document the methods we call in other libraries > (libpng has its own documentation for this method, FWIW). As a compromise, I've > added a comment the first time I use it. > > You also made me question the fact that read_header set the progressive_ptr, and > AutoCleanPng uses it. I've moved that instance into AutoCleanPng. Acknowledged. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:392: // FIXME: Make it possible to continue decoding all rows? On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:40, msarett wrote: > > What does this mean? > > This comment is probably out of place here. Removed. > > (FWIW, it means that I think it would be nice to have a way for the client to > say "I want all rows, incrementally". I imagined they might do that by calling > getPixels(), but we don't support that yet (a little more thoughts at [1]). If > we were to add that support, and continue calling through decodeAllRows, we > would need to not reset this here.) > > [1] > https://docs.google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_Cu8DF57KNtUmg... Acknowledged. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:401: if (rowsDecoded) { On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:41, msarett wrote: > > Should we set rowDecoded even if it's a success? > > I don't think so. On success the client need not read this variable. > > FWIW, onGetPixels already behaves this way. My comment in the header does not > specify this, though, so I have updated it. Acknowledged. https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:542: *rowsDecoded = fLinesDecoded; On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:40, msarett wrote: > > This is what we want for filling... But is it weird for a client to think > that > > we have decoded most/all of the rows when there are more interlaced passes to > > come. > > Perhaps weird, but I don't know why else the client would want to know how many > rows were decoded. > > I've updated the comment in the header to discuss this behavior. Perhaps I > should go further and change the name to rowsInitialized? Thanks for the comment. I don't feel strongly about the name. https://codereview.chromium.org/1997703003/diff/60001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:158: if (kIndex_8_SkColorType == info.colorType()) { \ nit: line up slash? https://codereview.chromium.org/1997703003/diff/60001/tests/CodecPartial.cpp File tests/CodecPartial.cpp (right): https://codereview.chromium.org/1997703003/diff/60001/tests/CodecPartial.cpp#... tests/CodecPartial.cpp:111: ERRORF(r, "Failed to start scanline decode\n"); nit: scanline -> incremental
https://codereview.chromium.org/1997703003/diff/60001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:158: if (kIndex_8_SkColorType == info.colorType()) { \ On 2016/05/20 18:21:38, msarett wrote: > nit: line up slash? D'oh! edited this after putting the slash there. Done. https://codereview.chromium.org/1997703003/diff/60001/tests/CodecPartial.cpp File tests/CodecPartial.cpp (right): https://codereview.chromium.org/1997703003/diff/60001/tests/CodecPartial.cpp#... tests/CodecPartial.cpp:111: ERRORF(r, "Failed to start scanline decode\n"); On 2016/05/20 18:21:38, msarett wrote: > nit: scanline -> incremental Done.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new scanline decoding API for SkCodec, since the existing one
is not easily compatible with using png_process_data. The new API uses
callbacks to determine where and whether to decode a row into the output.
The plan is to transition the other implementations from the old API to
the new one, then delete the old one (startScanlineDecode and get/skip-
Scanlines).
In the new API, the client specifies which rows to decode, and a
callback for telling the SkCodec where to write the result (or NULL to
skip).
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fit kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
FIXME: Need to update BMP to make ICO work properly.
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
Mike proposed we remove the callback from the public API. Instead, we use a sampleY to decide which rows to include and which rows to skip. PTAL at patch set 7 https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:453: png_error(this->png_ptr(), "Fake error to stop decoding scanlines."); On 2016/05/20 16:51:59, scroggo_chromium wrote: > On 2016/05/20 15:04:41, msarett wrote: > > Can we pass more info to this call so, when we catch it in process data, we > know > > that we are the ones who called it? > > Not to png_error, but we could call longjmp directly, which would allow us to > pass a special value. (png_error calls a method we specify. We make it call > sk_error_fn, which arbitrarily uses the value 1. We could specify 1 there and > something else here.) Patch set 6 calls longjmp directly, and uses different values. I turned on SkCodecPrintf, and saw a lot of output from all those png_errors, so I switched to longjmp. As long as I was doing that switch, I used different values for actual error versus stop decoding. PTAL. > > > > > Will that change how we handle it? > > I'm not sure... > > > > > I'm happy for this to be a follow up. > > sgtm.
Cool, I like this better than the first version. https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:235: SkCodec::Result SkCodec::startIncrementalDecode(const SkImageInfo& info, void* pixels, size_t rowBytes, nit: 100 chars https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:158: switch (setjmp(png_jmpbuf(fPng_ptr))) { Nice! https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkSampler.h File src/codec/SkSampler.h (right): https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkSampler.h#... src/codec/SkSampler.h:42: return row % fSampleY == 0; Is this correct? I think we don't start at row 0... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... Maybe this would be useful? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:235: SkCodec::Result SkCodec::startIncrementalDecode(const SkImageInfo& info, void* pixels, size_t rowBytes, On 2016/05/24 13:14:07, msarett wrote: > nit: 100 chars Done. https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkSampler.h File src/codec/SkSampler.h (right): https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkSampler.h#... src/codec/SkSampler.h:42: return row % fSampleY == 0; On 2016/05/24 13:14:07, msarett wrote: > Is this correct? I think we don't start at row 0... I believe it's used correctly by SkPngCodec, but it is unclear. Note how we actually pass fLinesDecoded, before incrementing it, so we start with zero, even though we may not be decoding row 0. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > Maybe this would be useful? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... We could use that method, although from the current call site, I don't know the scaledDim. I could plumb it in though. I think rowNeeded would work, but maybe needs some better documentation? PTAL at the new comment. It might be a tad circular though... I guess the ugly part is that once we start supporting bottom up BMPs through the new path we won't be able to just use fLinesDecoded; we'll need to something more like is_coord_necessary.
https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkSampler.h File src/codec/SkSampler.h (right): https://codereview.chromium.org/1997703003/diff/120001/src/codec/SkSampler.h#... src/codec/SkSampler.h:42: return row % fSampleY == 0; On 2016/05/24 14:24:50, scroggo wrote: > On 2016/05/24 13:14:07, msarett wrote: > > Is this correct? I think we don't start at row 0... > > I believe it's used correctly by SkPngCodec, but it is unclear. Note how we > actually pass fLinesDecoded, before incrementing it, so we start with zero, even > though we may not be decoding row 0. > > > Gotcha thanks. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > > > Maybe this would be useful? > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > We could use that method, although from the current call site, I don't know the > scaledDim. I could plumb it in though. I think rowNeeded would work, but maybe > needs some better documentation? PTAL at the new comment. It might be a tad > circular though... > New comment looks good. > I guess the ugly part is that once we start supporting bottom up BMPs through > the new path we won't be able to just use fLinesDecoded; we'll need to something > more like is_coord_necessary. It's fine with me if we figure this out when we get here.
Mike, can you take another look at the API?
api lgtm https://codereview.chromium.org/1997703003/diff/140001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/140001/include/codec/SkCodec.... include/codec/SkCodec.h:375: Result startIncrementalDecode(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, The first 3 parameters could be SkPixmap ...
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
FIXME: Need to update BMP to make ICO work properly.
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
FIXME: Need to update BMP to make ICO work properly.
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true
==========
scroggo@google.com changed reviewers: + scroggo@google.com
Matt, please take another look. I've added a workaround for BMP in ICO, and fixed a bug where SkSampledCodec would need to rewind in the case of falling back to scanline decoding. I've added a test for the latter (which would've broken Android). The former should be caught by running DM (found it will running DM on resources/color_wheel.ico). https://codereview.chromium.org/1997703003/diff/140001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/140001/include/codec/SkCodec.... include/codec/SkCodec.h:375: Result startIncrementalDecode(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, On 2016/05/24 15:11:38, reed1 wrote: > The first 3 parameters could be SkPixmap ... +1. I've been thinking about that for getPixels as well. I'll follow up on that in a separate CL. https://codereview.chromium.org/1997703003/diff/140001/include/codec/SkCodec.... include/codec/SkCodec.h:687: void updateCurrScanline(int newY) { fCurrScanline = newY; } Forgot to remove this last time... Gone now, in patch set 9 and onward. https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkIcoCodec.cpp File src/codec/SkIcoCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkIcoCodec.c... src/codec/SkIcoCodec.cpp:344: if (embeddedCodec->startScanlineDecode(dstInfo, nullptr, In patch set 9, I just called dimensionsSupported(). That may end up being correct, but I think this is the safer thing to do. Either one is a workaround, which will be removed once BMP supports incremental.
lgtm https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:286: } else if (kUnimplemented == result) { This code is temporary, right? Until we finish implementing? I'm guessing it's necessary because rewind() fails if there is nothing to rewind? Maybe add a FIXME? https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkIcoCodec.cpp File src/codec/SkIcoCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkIcoCodec.c... src/codec/SkIcoCodec.cpp:344: if (embeddedCodec->startScanlineDecode(dstInfo, nullptr, On 2016/05/26 16:10:58, scroggo wrote: > In patch set 9, I just called dimensionsSupported(). That may end up being > correct, but I think this is the safer thing to do. Either one is a workaround, > which will be removed once BMP supports incremental. Acknowledged.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
FIXME: Need to update BMP to make ICO work properly.
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/180001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:286: } else if (kUnimplemented == result) { On 2016/05/26 16:45:22, msarett wrote: > This code is temporary, right? Until we finish implementing? It is only temporarily necessary. It's not necessarily wrong to leave this unimplemented for some subclasses (e.g. webp, raw), and those subclasses won't need to rewind after returning unimplemented. But that's not a case that will happen in practice. > > I'm guessing it's necessary because rewind() fails if there is nothing to > rewind? No, the problem is that rewind() may just fail to rewind. The key use case here is on Android, using an arbitrary Java InputStream, and sampling. getAndroidPixels() will attempt to incremental decode, which will return unimplemented. Without this line here, the next call to startScanlineDecode() will attempt to rewind. Android only buffers the first MinBufferedBytesNeeded(), so it will fail outright. > > Maybe add a FIXME? Done.
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com, msarett@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/220001
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 scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps240001 (title: "Fix conversion error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps260001 (title: "Use the correct Options object")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkGifCodec.cpp File src/codec/SkGifCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkGifCodec.c... src/codec/SkGifCodec.cpp:523: return this->prepareToDecode(dstInfo, inputColorPtr, inputColorCount, opts); https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX... caught an interesting bug - we should've been using "opts", which was passed into this function, but instead we were using this->options(), which would have gotten updated to "opts" after this call returned. (Maybe the order is confusing and we should fix it and/or safeguard it? I don't see any other callers of options() that appear to be making this mistake though.) This bug has been around a while*, and definitely went into N. The good news is, we don't support fSubset in GIF. The only other Option is zero initialized. Fortunately, Android doesn't reuse the same SkCodec (except for BRD, which doesn't support GIF), so it will always reuse the default options, and the default options include kNo_ZeroInitialized. This means that for GIF, we won't take advantage of zero-initialized memory. But that's not so terrible. * Since scanline decoding was added to GIF in crrev.com/1305123002
https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkGifCodec.cpp File src/codec/SkGifCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkGifCodec.c... src/codec/SkGifCodec.cpp:523: return this->prepareToDecode(dstInfo, inputColorPtr, inputColorCount, opts); On 2016/05/26 20:25:10, scroggo wrote: > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX... > caught an interesting bug - we should've been using "opts", which was passed > into this function, but instead we were using this->options(), which would have > gotten updated to "opts" after this call returned. (Maybe the order is confusing > and we should fix it and/or safeguard it? I don't see any other callers of > options() that appear to be making this mistake though.) > > This bug has been around a while*, and definitely went into N. The good news is, > we don't support fSubset in GIF. The only other Option is zero initialized. > Fortunately, Android doesn't reuse the same SkCodec (except for BRD, which > doesn't support GIF), so it will always reuse the default options, and the > default options include kNo_ZeroInitialized. This means that for GIF, we won't > take advantage of zero-initialized memory. But that's not so terrible. > > * Since scanline decoding was added to GIF in crrev.com/1305123002 That's too bad. Thanks for pointing this out.
Matt, PTAL at the latest, which disables SkPngCodec with older versions of libpng https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:792: png_process_data_pause(fPng_ptr, 1); CMake build [1] is breaking with /b/work/skia/src/codec/SkPngCodec.cpp: In member function 'void AutoCleanPng::infoCallback()': /b/work/skia/src/codec/SkPngCodec.cpp:792:39: error: 'png_process_data_pause' was not declared in this scope png_process_data_pause(fPng_ptr, 1); IIUC, our CMake build will just use the version of libpng that is installed on the machine. This one happens to have an older version that does not have png_process_data_pause. This implementation requires png_process_data_pause in order to work, or the ability to rewind the stream (further - we already require at least some limited rewinding so that we can choose the format). Here are some options I see to solve this: - Don't support PNG with older versions of libpng (easiest option) - (Could be combined with upgrading the bot's version of libpng) - Require being able to rewind further on older versions of libpng - Could be pretty straightforward, but the client will probably just always fail to decode PNG - Don't use an SkStream in SkCodec - I've been thinking about using something more like SegmentReader in Blink, which would allow some limited rewinding that could support not being able to call png_process_data_pause. It's a big change, though, and I think it makes more sense to make that switch after all the SkCodecs support suspending. I toyed with starting with the old input method (png_read_info which calls SkStream::read on exactly the bytes it needs) and then switching to png_process_data for the rows. This doesn't work, though, because png_process_data wants to start from the beginning. Latest patch set goes with the first option. [1] https://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x8...
The CQ bit was checked by scroggo@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/1997703003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
lgtm https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/260001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:792: png_process_data_pause(fPng_ptr, 1); On 2016/05/26 21:53:32, scroggo wrote: > CMake build [1] is breaking with > > /b/work/skia/src/codec/SkPngCodec.cpp: In member function 'void > AutoCleanPng::infoCallback()': > /b/work/skia/src/codec/SkPngCodec.cpp:792:39: error: 'png_process_data_pause' > was not declared in this scope > png_process_data_pause(fPng_ptr, 1); > > IIUC, our CMake build will just use the version of libpng that is installed on > the machine. This one happens to have an older version that does not have > png_process_data_pause. > > This implementation requires png_process_data_pause in order to work, or the > ability to rewind the stream (further - we already require at least some limited > rewinding so that we can choose the format). Here are some options I see to > solve this: > > - Don't support PNG with older versions of libpng (easiest option) > - (Could be combined with upgrading the bot's version of libpng) > - Require being able to rewind further on older versions of libpng > - Could be pretty straightforward, but the client will probably just always > fail to decode PNG > - Don't use an SkStream in SkCodec > - I've been thinking about using something more like SegmentReader in Blink, > which would allow some limited rewinding > that could support not being able to call png_process_data_pause. It's a big > change, though, and I think it makes more > sense to make that switch after all the SkCodecs support suspending. > > I toyed with starting with the old input method (png_read_info which calls > SkStream::read on exactly the bytes it needs) and then switching to > png_process_data for the rows. This doesn't work, though, because > png_process_data wants to start from the beginning. > > Latest patch set goes with the first option. > > > [1] > https://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x8... First option sounds good to me. Looks like libpng has to be really, really old to not have this function.
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com, msarett@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps340001 (title: "Don't compare color space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/340001
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/2023103002/ by scroggo@google.com. The reason for reverting is: This is failing tests and then crashing on Google3 [1]. The crashes are fixed by crrev.com/2026873002, but to fix the builder we'll need to upgrade its version of libpng. [1] https://sponge.corp.google.com/target?id=e545ef55-4da4-4931-9524-1ac92acb61b1....
Message was sent while issue was closed.
Patchset #19 (id:360001) has been deleted
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
==========
Matt, please take another look. I've used a different workaround this time, to try to get the Google3 auto-roller bot to work. We don't have a trybot for Google3, so I'm going to go ahead and land and verify that it passes. https://codereview.chromium.org/1997703003/diff/460001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/460001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:782: fDecodedBounds = fStream->move(-fPng_ptr->buffer_size); My previous workaround simply dropped support for earlier versions of libpng. While I think that is the right way to go, we have a builder that uses an older version. I filed a bug to upgrade that (skbug.com/5368), but I don't want to be held up by that. This version happens to work in our tests, since we only use streams that can be move()ed (except for one case that I have disabled), so I expect it will succeed on the bots.
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps460001 (title: "Add explanatory comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/460001
On 2016/06/01 14:04:00, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1997703003/460001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1997703003/460001 lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps480001 (title: "Add missing override")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/480001
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Committed:
https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
==========
Message was sent while issue was closed.
Committed patchset #24 (id:480001) as https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
Message was sent while issue was closed.
A revert of this CL (patchset #24 id:480001) has been created in https://codereview.chromium.org/2026383002/ by scroggo@google.com. The reason for reverting is: Still breaking Google3 e.g.: https://sponge.corp.google.com/target?id=9261d31b-34fc-4f0f-981e-f92f7c5cea2c... https://test.corp.google.com/ui#cl=123773095&flags=CAMQBQ==&id=OCL:123773095:....
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Committed:
https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Committed:
https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
==========
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps520001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997703003/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997703003/520001
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Committed:
https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Committed:
https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
Committed:
https://skia.googlesource.com/skia/+/6fb2391b2cc83ee2160b4e994faa8128975acc1f
==========
Message was sent while issue was closed.
Committed patchset #26 (id:520001) as https://skia.googlesource.com/skia/+/6fb2391b2cc83ee2160b4e994faa8128975acc1f
Message was sent while issue was closed.
A revert of this CL (patchset #26 id:520001) has been created in https://codereview.chromium.org/2044573002/ by scroggo@google.com. The reason for reverting is: Still causing problems in Google3, e.g. https://test.corp.google.com/ui#cl=124138817&flags=CAMQBQ==&id=OCL:124138817:....
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/a4b09a117d4d1ba5dda372e6a2323e653766539e
Committed:
https://skia.googlesource.com/skia/+/30e78c9737ff4861dc4e3fa1e4cd010680ed6965
Committed:
https://skia.googlesource.com/skia/+/6fb2391b2cc83ee2160b4e994faa8128975acc1f
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
- Make initializeSwizzler return a bool instead of an SkCodec::Result
(It only returned kSuccess or kInvalidInput anyway)
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
- Set fDstInfo and fOptions in getPixels(). This may not be necessary
for all implementations, but it simplifies things for SkPngCodec.
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
Matt, please take another look. (API unchanged from prior approvals.) I've rebased/integrated your xform changes. The general gist is, where I used to call swizzle(), I now call applyXformRow. I also moved the last couple parameters into member variables.
Sorry, it's been a long time since I looked at this. I probably commented on some things that are not new changes. https://codereview.chromium.org/1997703003/diff/680001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1997703003/diff/680001/include/codec/SkCodec.... include/codec/SkCodec.h:724: // These fields are only meaningful during scanline decodes. nit: scanline or incremental decodes https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (left): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:364: fStorage.reset(SkAlign4(fSrcRowBytes)); Cool! Now we decode into memory allocated by libpng instead of our own storage buffer? https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:1107: this->initializeXformAlphaAndWidth(); Why can't you call this in startIncrementalDecode()? https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:1123: SkCodec* outCodec = nullptr; Why this change? Did you intend to use a single return statement? https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... src/codec/SkSampledCodec.cpp:223: incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1; Why are we doing this again?
PTAL
Patchset:
35: Addresses comments (only one change, plus a couple of
minor things I noticed
36: Fix a bug your comment made me think of
37: Rebase
https://codereview.chromium.org/1997703003/diff/680001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):
https://codereview.chromium.org/1997703003/diff/680001/include/codec/SkCodec....
include/codec/SkCodec.h:724: // These fields are only meaningful during scanline
decodes.
On 2016/09/14 13:21:06, msarett wrote:
> nit: scanline or incremental decodes
Actually, fStartedIncrementalDecode is generally meaningful, and fCurrScanline
is only used during scanline decode. I've made this more clear.
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (left):
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.c...
src/codec/SkPngCodec.cpp:364: fStorage.reset(SkAlign4(fSrcRowBytes));
On 2016/09/14 13:21:06, msarett wrote:
> Cool! Now we decode into memory allocated by libpng instead of our own
storage
> buffer?
Yes - png_process_data decodes into a buffer it passes to our callback. I don't
know whether they're any more efficient than we could be, but it's one less
thing for us to keep track of.
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.c...
src/codec/SkPngCodec.cpp:1107: this->initializeXformAlphaAndWidth();
On 2016/09/14 13:21:06, msarett wrote:
> Why can't you call this in startIncrementalDecode()?
I almost called it inside initializeXforms (which is called by
startIncrementalDecode) but the reason incremental decoding needs to be split up
is because SkSampledCodec needs to set the sampling rate after calling
startIncrementalDecode. That changes the swizzle width (and possibly create's a
swizzler/sampler), so we need to do this after that. (We could also split up the
alpha type and width, but I kind of like keeping them together...)
As an alternative, we could make setting the sampling rate call back into the
codec to tell it to call this method again, although that adds some more
complexity.
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkPngCodec.c...
src/codec/SkPngCodec.cpp:1123: SkCodec* outCodec = nullptr;
On 2016/09/14 13:21:06, msarett wrote:
> Why this change? Did you intend to use a single return statement?
I wanted to make sure that if I got a codec out of here it wasn't garbage.
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod...
File src/codec/SkSampledCodec.cpp (right):
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod...
src/codec/SkSampledCodec.cpp:223: incrementalSubset.fBottom = startY +
(dstHeight - 1) * sampleY + 1;
On 2016/09/14 13:21:06, msarett wrote:
> Why are we doing this again?
Modifying the top and bottom? Because unlike the scanline decoder, the
incremental decoder does not have a way to skip individual scanlines. Instead,
it tells it any lines to skip at the beginning and the end (and then uses
sampleY to skip any lines in the middle).
lgtm https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... src/codec/SkSampledCodec.cpp:223: incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1; On 2016/09/14 14:54:13, scroggo_chromium wrote: > On 2016/09/14 13:21:06, msarett wrote: > > Why are we doing this again? > > Modifying the top and bottom? Because unlike the scanline decoder, the > incremental decoder does not have a way to skip individual scanlines. Instead, > it tells it any lines to skip at the beginning and the end (and then uses > sampleY to skip any lines in the middle). Sorry let me rephrase. Why do we need to do this math to determine the bottom of the subset? "startY + (dstHeight - 1) * sampleY + 1"
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... src/codec/SkSampledCodec.cpp:223: incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1; On 2016/09/14 15:39:54, msarett wrote: > On 2016/09/14 14:54:13, scroggo_chromium wrote: > > On 2016/09/14 13:21:06, msarett wrote: > > > Why are we doing this again? > > > > Modifying the top and bottom? Because unlike the scanline decoder, the > > incremental decoder does not have a way to skip individual scanlines. Instead, > > it tells it any lines to skip at the beginning and the end (and then uses > > sampleY to skip any lines in the middle). > > Sorry let me rephrase. Why do we need to do this math to determine the bottom > of the subset? > "startY + (dstHeight - 1) * sampleY + 1" dstHeight is the height of the output (this was part of the SkAndroidCodec, which promises to scale to the size of the SkImageInfo provided). But now we're going to pass a subset to an SkCodec, which interprets it as a subset into the original image. So we need to compute the size of a subset in the original image that, when sampled with our algorithm, will create an image of size dstHeight. Does that make sense? Do you expect to see different math/something simpler?
https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... src/codec/SkSampledCodec.cpp:223: incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1; On 2016/09/14 17:43:00, scroggo wrote: > On 2016/09/14 15:39:54, msarett wrote: > > On 2016/09/14 14:54:13, scroggo_chromium wrote: > > > On 2016/09/14 13:21:06, msarett wrote: > > > > Why are we doing this again? > > > > > > Modifying the top and bottom? Because unlike the scanline decoder, the > > > incremental decoder does not have a way to skip individual scanlines. > Instead, > > > it tells it any lines to skip at the beginning and the end (and then uses > > > sampleY to skip any lines in the middle). > > > > Sorry let me rephrase. Why do we need to do this math to determine the bottom > > of the subset? > > "startY + (dstHeight - 1) * sampleY + 1" > > dstHeight is the height of the output (this was part of the SkAndroidCodec, > which promises to scale to the size of the SkImageInfo provided). But now we're > going to pass a subset to an SkCodec, which interprets it as a subset into the > original image. So we need to compute the size of a subset in the original image > that, when sampled with our algorithm, will create an image of size dstHeight. > Does that make sense? Do you expect to see different math/something simpler? My latest interpretation: We *could* get away with using "subsetY and "subsetHeight" (calculated above), but this code gives us tighter bounds on the subset - meaning that we don't include rows that we'll skip anyway due to sampling. LGTM, maybe a comment? https://codereview.chromium.org/1997703003/diff/730001/src/codec/SkSampler.h File src/codec/SkSampler.h (right): https://codereview.chromium.org/1997703003/diff/730001/src/codec/SkSampler.h#... src/codec/SkSampler.h:44: return row % fSampleY == 0; I actually think for this to work properly, it depends on the subset being specified with tight bounds... Ex: We always want to keep row 0, when the subset is specified this way.
Matt, PTAL. I've further simplified applyXformRow, and worked around the memory corruption problem, which appears to be a bug in fill that I uncovered. https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... File src/codec/SkSampledCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/680001/src/codec/SkSampledCod... src/codec/SkSampledCodec.cpp:223: incrementalSubset.fBottom = startY + (dstHeight - 1) * sampleY + 1; On 2016/09/14 18:16:05, msarett wrote: > On 2016/09/14 17:43:00, scroggo wrote: > > On 2016/09/14 15:39:54, msarett wrote: > > > On 2016/09/14 14:54:13, scroggo_chromium wrote: > > > > On 2016/09/14 13:21:06, msarett wrote: > > > > > Why are we doing this again? > > > > > > > > Modifying the top and bottom? Because unlike the scanline decoder, the > > > > incremental decoder does not have a way to skip individual scanlines. > > Instead, > > > > it tells it any lines to skip at the beginning and the end (and then uses > > > > sampleY to skip any lines in the middle). > > > > > > Sorry let me rephrase. Why do we need to do this math to determine the > bottom > > > of the subset? > > > "startY + (dstHeight - 1) * sampleY + 1" > > > > dstHeight is the height of the output (this was part of the SkAndroidCodec, > > which promises to scale to the size of the SkImageInfo provided). But now > we're > > going to pass a subset to an SkCodec, which interprets it as a subset into the > > original image. So we need to compute the size of a subset in the original > image > > that, when sampled with our algorithm, will create an image of size dstHeight. > > Does that make sense? Do you expect to see different math/something simpler? > > My latest interpretation: > We *could* get away with using "subsetY and "subsetHeight" (calculated above), > but this code gives us tighter bounds on the subset - meaning that we don't > include rows that we'll skip anyway due to sampling. > > LGTM, maybe a comment? Done. https://codereview.chromium.org/1997703003/diff/730001/src/codec/SkSampler.h File src/codec/SkSampler.h (right): https://codereview.chromium.org/1997703003/diff/730001/src/codec/SkSampler.h#... src/codec/SkSampler.h:44: return row % fSampleY == 0; On 2016/09/14 18:16:05, msarett wrote: > I actually think for this to work properly, it depends on the subset being > specified with tight bounds... > > Ex: We always want to keep row 0, when the subset is specified this way. Agreed. I think the comment describes that? ("starting with the first row used in the output") https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:225: //fOptions = *options; I haven't figured out what's going on here - the fill code is "working" without me setting this (so we're using an old value for fOptions - which matters because fillIncompleteImage uses fSubset), but when I update it (to the correct subset, I think) we get a memory corruption. (It's pretty consistent, too. Besides seeing it in https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-Clang-GCE-CPU-A..., you can reproduce locally with: ./out/Debug/dm --resourcePath resources/ --images inc1.webp where inc1.webp is in our test images.)
lgtm https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:225: //fOptions = *options; On 2016/09/14 22:01:41, scroggo_chromium wrote: > I haven't figured out what's going on here - the fill code is "working" without > me setting this (so we're using an old value for fOptions - which matters > because fillIncompleteImage uses fSubset), but when I update it (to the correct > subset, I think) we get a memory corruption. (It's pretty consistent, too. > Besides seeing it in > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-Clang-GCE-CPU-A..., > you can reproduce locally with: > > ./out/Debug/dm --resourcePath resources/ --images inc1.webp > > where inc1.webp is in our test images.) I'll take a look.
Woohoo! Trybots passed! https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:225: //fOptions = *options; On 2016/09/14 22:11:56, msarett wrote: > On 2016/09/14 22:01:41, scroggo_chromium wrote: > > I haven't figured out what's going on here - the fill code is "working" > without > > me setting this (so we're using an old value for fOptions - which matters > > because fillIncompleteImage uses fSubset), but when I update it (to the > correct > > subset, I think) we get a memory corruption. (It's pretty consistent, too. > > Besides seeing it in > > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-Clang-GCE-CPU-A..., > > you can reproduce locally with: > > > > ./out/Debug/dm --resourcePath resources/ --images inc1.webp > > > > where inc1.webp is in our test images.) > > I'll take a look. This is an absolute mess (and it's my fault). We have three different ways of choosing the "width" to use to fill. Here they are, by priority. (1) Whatever the "swizzler" thinks the width is. (2) Whatever the "subset" width is. (3) Whatever the "dstInfo" width is. (1) always seems to work, the swizzler seems to be an absolute source of truth on the real width. We run into problems when we don't have a swizzler... (2) SkJpegCodec needs to use the "subset" width on scanline decodes. Because jpegs can do partial scanlines, we may have a case where we have no swizzler, but are performing a subset decode. In this case, we should fill with the "subset" width. (3) SkWebpCodec needs to *not* use the "subset" width. The subset will be specified in terms of the original image (not the scaled image). We want to fill with the scaled subset width, which will be stored in dstInfo. So, by some crazy luck, everything works now - because we only set the subset on scanline decodes (when jpeg needs the subset width), but not on regular decodes (when webp needs the dstInfo.width()). My recommendation would be to not change anything in this CL :). ----------------------------------------------- How should we actually fix this? I think we need a consistent interpretation of what dstInfo and fSubset mean across the entry points to SkCodec. Potentially that could be done by changing SkWebpCodec to take the subset in terms of the scaled image (which, of course, does not match the libwebp API)? https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:225: //fOptions = *options; On 2016/09/14 22:11:56, msarett wrote: > On 2016/09/14 22:01:41, scroggo_chromium wrote: > > I haven't figured out what's going on here - the fill code is "working" > without > > me setting this (so we're using an old value for fOptions - which matters > > because fillIncompleteImage uses fSubset), but when I update it (to the > correct > > subset, I think) we get a memory corruption. (It's pretty consistent, too. > > Besides seeing it in > > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-Clang-GCE-CPU-A..., > > you can reproduce locally with: > > > > ./out/Debug/dm --resourcePath resources/ --images inc1.webp > > > > where inc1.webp is in our test images.) > > I'll take a look. Additional side note: We don't have any incomplete pngs on the bots. This is bad. Running locally, it looks like codec currently "works" on incomplete pngs, but doesn't work after your CL. Haven't tried to figure out why yet. I would suggest that we land this, then add incomplete pngs (with bug fixes).
Matt, PTAL
Patchsets:
42: I accidentally wasn't testing complete PNG images in
CodecTest. Fix that.
43: Uninteresting cleanups, but feel free to look at.
44: Not sure how I feel about this one. Google3 has an old
version that does not have png_process_data_pause.
Blink's PNGImageDecoder appears to have a workaround[1],
but I haven't been able to get it work. (Ours is a
little different because may not be able to go
backwards in our input, but the workaround I went with
does not necessarily either.) Instead I just reread the
header. It's ugly, but maybe it's good enough until we
get Google3 updated.
[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):
https://codereview.chromium.org/1997703003/diff/810001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:225: //fOptions = *options;
On 2016/09/14 23:20:20, msarett wrote:
> On 2016/09/14 22:11:56, msarett wrote:
> > On 2016/09/14 22:01:41, scroggo_chromium wrote:
> > > I haven't figured out what's going on here - the fill code is "working"
> > without
> > > me setting this (so we're using an old value for fOptions - which matters
> > > because fillIncompleteImage uses fSubset), but when I update it (to the
> > correct
> > > subset, I think) we get a memory corruption. (It's pretty consistent, too.
> > > Besides seeing it in
> > >
> >
>
https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-Clang-GCE-CPU-A...,
> > > you can reproduce locally with:
> > >
> > > ./out/Debug/dm --resourcePath resources/ --images inc1.webp
> > >
> > > where inc1.webp is in our test images.)
> >
> > I'll take a look.
>
> This is an absolute mess (and it's my fault). We have three different ways of
> choosing the "width" to use to fill. Here they are, by priority.
> (1) Whatever the "swizzler" thinks the width is.
> (2) Whatever the "subset" width is.
> (3) Whatever the "dstInfo" width is.
>
> (1) always seems to work, the swizzler seems to be an absolute source of truth
> on the real width. We run into problems when we don't have a swizzler...
>
> (2) SkJpegCodec needs to use the "subset" width on scanline decodes. Because
> jpegs can do partial scanlines, we may have a case where we have no swizzler,
> but are performing a subset decode. In this case, we should fill with the
> "subset" width.
>
> (3) SkWebpCodec needs to *not* use the "subset" width. The subset will be
> specified in terms of the original image (not the scaled image). We want to
> fill with the scaled subset width, which will be stored in dstInfo.
>
> So, by some crazy luck, everything works now - because we only set the subset
on
> scanline decodes (when jpeg needs the subset width), but not on regular
decodes
> (when webp needs the dstInfo.width()).
>
> My recommendation would be to not change anything in this CL :).
>
> -----------------------------------------------
>
> How should we actually fix this? I think we need a consistent interpretation
of
> what dstInfo and fSubset mean across the entry points to SkCodec. Potentially
> that could be done by changing SkWebpCodec to take the subset in terms of the
> scaled image (which, of course, does not match the libwebp API)?
That's certainly an option, and maybe it's not so bad...
https://codereview.chromium.org/1997703003/diff/830001/tests/CodecTest.cpp
File tests/CodecTest.cpp (left):
https://codereview.chromium.org/1997703003/diff/830001/tests/CodecTest.cpp#ol...
tests/CodecTest.cpp:424: check(r, "arrow.png", SkISize::Make(187, 312), true,
false, false);
(Moving your comment to a spot relevant to my point:)
> Additional side note:
>
> We don't have any incomplete pngs on the bots. This is bad. Running locally,
> it looks like codec currently "works" on incomplete pngs,
I think it does currently work - if you change false to true for incomplete
pngs, it passes.
> but doesn't work after
> your CL. Haven't tried to figure out why yet. I would suggest that we land
> this, then add incomplete pngs (with bug fixes).
Wait, what doesn't work/how does it fail? I think we should make it work before
checking this in (this is all about supporting incomplete, after all!). I'm
passing true for supportsIncomplete, and it does not fail the test (but maybe
the test doesn't check enough).
incrementalDecode does not fill, is that the problem? In DM's scanline mode, I
call fill outside of SkCodec, so that should cover that case. And SkAndroidCodec
(SkSampledCodec) fills, too. I think those are the only places where we test
incrementalDecode.
LGTM Yes everything inside #ifdef GOOGLE3_PNG_HACK is ugly... But I think it's justified - landing this makes it easier for us to continue making progress. https://codereview.chromium.org/1997703003/diff/830001/tests/CodecTest.cpp File tests/CodecTest.cpp (left): https://codereview.chromium.org/1997703003/diff/830001/tests/CodecTest.cpp#ol... tests/CodecTest.cpp:424: check(r, "arrow.png", SkISize::Make(187, 312), true, false, false); On 2016/09/15 22:34:54, scroggo_chromium wrote: > (Moving your comment to a spot relevant to my point:) > > > Additional side note: > > > > We don't have any incomplete pngs on the bots. This is bad. Running locally, > > it looks like codec currently "works" on incomplete pngs, > > I think it does currently work - if you change false to true for incomplete > pngs, it passes. > > > but doesn't work after > > your CL. Haven't tried to figure out why yet. I would suggest that we land > > this, then add incomplete pngs (with bug fixes). > > Wait, what doesn't work/how does it fail? I think we should make it work before > checking this in (this is all about supporting incomplete, after all!). I'm > passing true for supportsIncomplete, and it does not fail the test (but maybe > the test doesn't check enough). > > incrementalDecode does not fill, is that the problem? In DM's scanline mode, I > call fill outside of SkCodec, so that should cover that case. And SkAndroidCodec > (SkSampledCodec) fills, too. I think those are the only places where we test > incrementalDecode. I think it's a fill() problem. The problem I thought I saw (patched locally, we don't have this test on the bots) was running an incomplete png through AndroidCodecSrc and/or BRDCodecSrc where I think we'll try to fill. I'm fine if you want to investigate (or prove me wrong) in this CL, but I'm also fine if we want to land a separate change with this test (and any fixes that might need to go with it).
The CQ bit was checked by scroggo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scroggo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps870001 (title: "Support Google3...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
The CQ bit was checked by scroggo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1997703003/#ps890001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
- Set fDstInfo and fOptions in getPixels(). This may not be necessary
for all implementations, but it simplifies things for SkPngCodec.
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========
to
==========
Make SkPngCodec decode progressively.
This is a step towards using SkCodec in Chromium, where progressive
decoding is necessary.
Switch from using png_read_row (which expects all the data to be
available) to png_process_data, which uses callbacks when rows are
available.
Create a new API for SkCodec, which supports progressive decoding and
scanline decoding. Future changes will switch the other clients off of
startScanlineDecode and get/skip-Scanlines to the new API.
Remove SkCodec::kNone_ScanlineOrder, which was only used for interlaced
PNG images. In the new API, interlaced PNG fits kTopDown. Also remove
updateCurrScanline(), which was only used by the old implementation for
interlaced PNG.
DMSrcSink:
- In CodecSrc::kScanline_Mode, use the new method for scanline decoding
for the supported formats (just PNG and PNG-in-ICO for now).
fuzz.cpp:
- Remove reference to kNone_ScanlineOrder
SkCodec:
- Add new APIs:
- startIncrementalDecode
- incrementalDecode
- Remove kNone_SkScanlineOrder and updateCurrScanline()
- Set fDstInfo and fOptions in getPixels(). This may not be necessary
for all implementations, but it simplifies things for SkPngCodec.
SkPngCodec:
- Implement new APIs
- Switch from sk_read_fn/png_read_row etc to png_process_data
- Expand AutoCleanPng's role to decode the header and create the
SkPngCodec
- Make the interlaced PNG decoder report how many lines were
initialized during an incomplete decode
SkIcoCodec:
- Implement the new APIs; supported for PNG in ICO
SkSampledCodec:
- Call the new method for decoding scanlines, and fall back to the old
method if the new version is unimplemented
- Remove references to kNone_SkScanlineOrder
tests/CodecPartial:
- Add a test which decodes part of an image, then finishes the decode,
and compares it to the straightforward method
tests/CodecTest:
- Add a test which decodes all scanlines using the new method
- Repurpose the Codec_stripes test to decode using the new method in
sections rather than all at once
- In the method check(), add a parameter for whether the image supports
the new method of scanline decoding, and be explicit about whether an
image supports incomplete
- Test incomplete PNG decodes. We should have been doing it anyway for
non-interlaced (except for an image that is too small - one row), but
the new method supports interlaced incomplete as well
- Make test_invalid_parameters test the new method
- Add a test to ensure that it's safe to fall back to scanline decoding without
rewinding
BUG=skia:4211
The new version was generally faster than the old version (but not significantly
so).
Some raw performance differences can be found at
https://docs.google.com/a/google.com/spreadsheets/d/1Gis3aRCEa72qBNDRMgGDg3jD...
Design doc can be found at
https://docs.google.com/a/google.com/document/d/11Mn8-ePDKwVEMCjs3nWwSjxcSpJ_...
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
Committed:
https://skia.googlesource.com/skia/+/8e6c7ada5a4c5a950cded765d14d3e6906acdc79
==========
Message was sent while issue was closed.
Committed patchset #45 (id:890001) as https://skia.googlesource.com/skia/+/8e6c7ada5a4c5a950cded765d14d3e6906acdc79 |
