Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1472933002:
Make SkAndroidCodec support ico (Closed)
DescriptionMake SkAndroidCodec support ico
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/be8216a922241cc8f3ea3b813608fcb06936fde0
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Add a sampleX() getter to SkSwizzler #Patch Set 3 : Add onSkipScanlines() #Patch Set 4 : Rebase: Make this CL depend on previous CL #
Total comments: 21
Patch Set 5 : Response to comments #
Total comments: 6
Patch Set 6 : #Patch Set 7 : Update CodexTest - Ico now supports scanline decoding #
Total comments: 1
Patch Set 8 : ASAN fix #
Total comments: 4
Patch Set 9 : Response to comments #
Depends on Patchset: Messages
Total messages: 37 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
This isn't completely ready, but I thought I'd go ahead and upload in case you want to take a look. There's a few interesting things going on that I've noted in the comments. It's worth mentioning that I don't think we need to land this if we don't like it. AFAICT SkImageDecoder_libico doesn't support sampling. https://codereview.chromium.org/1472933002/diff/30001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1472933002/diff/30001/include/codec/SkCodec.h... include/codec/SkCodec.h:597: friend class SkIcoCodec; SkIcoCodec has to call fCodec->getSampler(). https://codereview.chromium.org/1472933002/diff/30001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/30001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:310: dstRow[x] &= alphaBit - 1; This is currently broken. We either need to know about sampling here or we need to handle decodeIcoMask() in the swizzler. https://codereview.chromium.org/1472933002/diff/30001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (left): https://codereview.chromium.org/1472933002/diff/30001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:511: const int currRow = this->INHERITED::nextScanline(); This is a bug, as you've pointed out in the past. What we really want is currScanline. What we are getting may be weird if the image is interlaced.
I decided to go ahead and support scanline decoding. I prefer this to checking if encodedFormat == kIco in a bunch of places in SkSampledCodec. I'm still happy to take a shot at that though if you feel this approach doesn't make sense.
On 2015/12/02 14:26:34, msarett wrote: > I decided to go ahead and support scanline decoding. > > I prefer this to checking if encodedFormat == kIco in a bunch of places in > SkSampledCodec. I'm still happy to take a shot at that though if you feel this > approach doesn't make sense. I think it makes sense. It just means we have more possibilities for regressions that we need to watch out for. ICO is probably a rarely used format on Android, so if there is a problem, it might not show up right away. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:247: const size_t andMaskRowBytes = SkAlign4(compute_row_bytes(this->getInfo().width(), 1)); IIUC, compute_row_bytes will return width * 8. Isn't that already SkAlign4? https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:263: SkMemoryStream* stream = (SkMemoryStream*) this->stream(); I think this is the correct assumption to make, but the wrong way to make it. If the assumption was wrong for some reason (i.e. we change the implementation elsewhere), I think this will fail mysteriously. (We would be treating some other kind of stream as an SkMemoryStream... actually, it looks like you still call only virtual functions on SkStream, so the cast is unnecessary?) I think a better way to handle this would be to call SkStream::getMemoryBase() and ASSERT that it returns a reasonable value (and maybe even ASSERT at creation if fInIco). You can then read that as an SkMemoryStream, so you'll never need to modify this->stream(). (This would be cleaner if/when we convert getMemoryBase to be a method returning an SkData, as proposed in the comments. Last I looked at that I think it was a big messy change to make...) It might be fine to just use this->stream() directly, but make sure you look at the return values of calls that might fail if the assumption is wrong. seek could fail, for example. So we should assert that seek returns true. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:274: // FIXME: If decodeIcoMask does not succeed, is there a way that we can You removed the return value from decodeIcoMask, which seems like it would be helpful. The tricky thing, of course, is that you indicate incomplete here by using the height. But the height is weird - we have partially decoded the entire height, but did not decode the mask for those lines. Furthermore, I think the height is used to tell SkCodec to fill the remaining lines. Do we want to fill those lines? We cannot really separate the two ideas (filling versus incomplete) as the code is written. What does Blink's decoder do? I'm guessing that it just stops when it runs out of data. Not sure that is the best behavior here, but something to consider. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:299: if (this->stream()->read(fSrcBuffer.get(), srcRowBytes) != srcRowBytes) { Should this be a private method? (I assume we do not want to calculate the rowBytes for each call; maybe store it as a const member variable?) It seems weird that we need to pass the srcRowBytes to this method https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:23: // FIXME: For PNG-in-ICO, we could technically support whichever What is preventing us from supporting this now? I think this might need to first figure out the size, and then based on the embedded codec to use, decide whether this is supported. (Which might make it tricky for me to land my virtual function version.) https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:373: // FIXME: This function will possibly return the wrong value if it is called I think it's more interesting than that. This function returns different values depending on which embedded codec is being used. Does anyone need to call getScanlineOrder outside of scanline decoding? Maybe we should explicitly state that getScanlineOrder is undefined before startScanlineDecode is called? https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libico.h File src/codec/SkCodec_libico.h (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.h:66: * is found, or we have reached the end of the SkTArray. SkTArray is an implementation detail that isn't necessary in this comment. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.h:88: SkCodec* fCodec; I think this should have a name that indicates that this is used for scanline decoding (or for an in progress decode). Maybe fCurrScanlineCodec? That's awfully cumbersome :( https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:166: int sampleX() const { return fSampleX; } Add a comment describing what this means?
Patchset #5 (id:110001) has been deleted
https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:247: const size_t andMaskRowBytes = SkAlign4(compute_row_bytes(this->getInfo().width(), 1)); On 2015/12/02 16:27:03, scroggo wrote: > IIUC, compute_row_bytes will return width * 8. Isn't that already SkAlign4? This will actually give us width / 8 (because there are 8 pixelsPerByte), which isn't necessarily aligned. I accidentally removed this at one point and it caused some failures. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:263: SkMemoryStream* stream = (SkMemoryStream*) this->stream(); On 2015/12/02 16:27:03, scroggo wrote: > I think this is the correct assumption to make, but the wrong way to make it. If > the assumption was wrong for some reason (i.e. we change the implementation > elsewhere), I think this will fail mysteriously. (We would be treating some > other kind of stream as an SkMemoryStream... actually, it looks like you still > call only virtual functions on SkStream, so the cast is unnecessary?) > > I think a better way to handle this would be to call SkStream::getMemoryBase() > and ASSERT that it returns a reasonable value (and maybe even ASSERT at creation > if fInIco). You can then read that as an SkMemoryStream, so you'll never need to > modify this->stream(). (This would be cleaner if/when we convert getMemoryBase > to be a method returning an SkData, as proposed in the comments. Last I looked > at that I think it was a big messy change to make...) > > It might be fine to just use this->stream() directly, but make sure you look at > the return values of calls that might fail if the assumption is wrong. seek > could fail, for example. So we should assert that seek returns true. I think you're right - the clearest thing is probably to get the memory base (and assert that it succeeds) and then operate on the memory. In addition, I actually decided to wrap the memory base in another stream, just because it is convenient to call stream->read() rather than check the length of the stream every time we access memory. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:274: // FIXME: If decodeIcoMask does not succeed, is there a way that we can On 2015/12/02 16:27:03, scroggo wrote: > You removed the return value from decodeIcoMask, which seems like it would be > helpful. The tricky thing, of course, is that you indicate incomplete here by > using the height. But the height is weird - we have partially decoded the entire > height, but did not decode the mask for those lines. > Yeah the return value from decodeIcoMask() could tell us incomplete. The reason I removed the return value is that I don't think we want to use it here. If we return less than the full height from decodeIcoMask() (and then return it here), we will fill some rows of pixels. I think the behavior we want is to just leave those pixels unmasked but not filled (though I guess it's arbitrary). > Furthermore, I think the height is used to tell SkCodec to fill the remaining > lines. Do we want to fill those lines? We cannot really separate the two ideas > (filling versus incomplete) as the code is written. > Yes I think this is the problem. We want to indicate that all of the lines are filled, but return kIncomplete. SkBmpRLECodec has a slightly similar problem with scanline decodes (skbug.com/4582). > What does Blink's decoder do? I'm guessing that it just stops when it runs out > of data. Not sure that is the best behavior here, but something to consider. AFAICT blink's decoder will leave the pixels unmasked (same as we do) since it will just stop when there is no more data. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:299: if (this->stream()->read(fSrcBuffer.get(), srcRowBytes) != srcRowBytes) { On 2015/12/02 16:27:03, scroggo wrote: > Should this be a private method? (I assume we do not want to calculate the > rowBytes for each call; maybe store it as a const member variable?) > > It seems weird that we need to pass the srcRowBytes to this method Agreed. I've made fAndMaskRowBytes a field. Given the latest change, I'm not sure it makes sense to make this a private method. We could have potentially created a method also used by decodeRows() (and passed rowBytes as a parameter). But it's a little messier now that I am passing in a stream to decodeIcoMask(). https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:23: // FIXME: For PNG-in-ICO, we could technically support whichever On 2015/12/02 16:27:03, scroggo wrote: > What is preventing us from supporting this now? I think this might need to first > figure out the size, and then based on the embedded codec to use, decide whether > this is supported. (Which might make it tricky for me to land my virtual > function version.) > Yes I think that's exactly what we need to do. Along with minor changes to NewFromStream() (to actually sometimes recommend opaque). Can we keep this for a later change? https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:373: // FIXME: This function will possibly return the wrong value if it is called On 2015/12/02 16:27:03, scroggo wrote: > I think it's more interesting than that. This function returns different values > depending on which embedded codec is being used. Does anyone need to call > getScanlineOrder outside of scanline decoding? Maybe we should explicitly state > that getScanlineOrder is undefined before startScanlineDecode is called? Yes you're right. The good news is that no one needs to call this outside of scanline decoding. We can add comments to SkCodec to explicitly state that this is undefined before startScanlineDecode is called. I think this unfortunate because this is the only case where it is undefined. And we actually went to the trouble of adding more stuff to ReadHeader() in SkGifCodec in order to avoid this problem (though there were other reasons too). However, I think this is the best option. Alternatively, if fCurrScanlineCodec is null, we could call chooseCodec() on this->getInfo()->dimensions() here and return the scanline order for whatever the "default codec" is. But that could still lead to confusion if the caller expects the scanline order to stay the same after calling startScanlineDecode(). https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libico.h File src/codec/SkCodec_libico.h (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.h:66: * is found, or we have reached the end of the SkTArray. On 2015/12/02 16:27:03, scroggo wrote: > SkTArray is an implementation detail that isn't necessary in this comment. Done. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.h:88: SkCodec* fCodec; On 2015/12/02 16:27:03, scroggo wrote: > I think this should have a name that indicates that this is used for scanline > decoding (or for an in progress decode). Maybe fCurrScanlineCodec? That's > awfully cumbersome :( Agreed. fCurrScanlineCodec it is...I can't think of anything better. https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:166: int sampleX() const { return fSampleX; } On 2015/12/02 16:27:03, scroggo wrote: > Add a comment describing what this means? Done.
lgtm https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:23: // FIXME: For PNG-in-ICO, we could technically support whichever On 2015/12/03 19:19:16, msarett wrote: > On 2015/12/02 16:27:03, scroggo wrote: > > What is preventing us from supporting this now? I think this might need to > first > > figure out the size, and then based on the embedded codec to use, decide > whether > > this is supported. (Which might make it tricky for me to land my virtual > > function version.) > > > > Yes I think that's exactly what we need to do. Along with minor changes to > NewFromStream() (to actually sometimes recommend opaque). > > Can we keep this for a later change? Sounds fine to me (maybe include a bug?) https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:373: // FIXME: This function will possibly return the wrong value if it is called On 2015/12/03 19:19:16, msarett wrote: > On 2015/12/02 16:27:03, scroggo wrote: > > I think it's more interesting than that. This function returns different > values > > depending on which embedded codec is being used. Does anyone need to call > > getScanlineOrder outside of scanline decoding? Maybe we should explicitly > state > > that getScanlineOrder is undefined before startScanlineDecode is called? > > Yes you're right. > > The good news is that no one needs to call this outside of scanline decoding. > > We can add comments to SkCodec to explicitly state that this is undefined before > startScanlineDecode is called. > > I think this unfortunate because this is the only case where it is undefined. > And we actually went to the trouble of adding more stuff to ReadHeader() in > SkGifCodec in order to avoid this problem (though there were other reasons too). > However, I think this is the best option. This may be the only case, but as you state above, it's not needed when it would be undefined. > > Alternatively, if fCurrScanlineCodec is null, we could call chooseCodec() on > this->getInfo()->dimensions() here and return the scanline order for whatever > the "default codec" is. But that could still lead to confusion if the caller > expects the scanline order to stay the same after calling startScanlineDecode(). Agreed. There is no reason to make an arbitrary guess in this situation. https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkBmpStandar... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:264: SkASSERT(this->stream()->hasLength()); I think this belongs above the call to getLength https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:266: SkASSERT(this->stream()->hasPosition()); Same here. Maybe put all the SkASSERTs together? I think that will make it easier to read. https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:167: * Getter for fSampleX. This line doesn't state anything that cannot be gleaned from looking at the code. I think this should say something about what it means - the swizzler is sampling every sampleX'th pixel (and discarding the rest).
Derek, can you take a look at the API? https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1472933002/diff/90001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:23: // FIXME: For PNG-in-ICO, we could technically support whichever On 2015/12/03 19:39:24, scroggo wrote: > On 2015/12/03 19:19:16, msarett wrote: > > On 2015/12/02 16:27:03, scroggo wrote: > > > What is preventing us from supporting this now? I think this might need to > > first > > > figure out the size, and then based on the embedded codec to use, decide > > whether > > > this is supported. (Which might make it tricky for me to land my virtual > > > function version.) > > > > > > > Yes I think that's exactly what we need to do. Along with minor changes to > > NewFromStream() (to actually sometimes recommend opaque). > > > > Can we keep this for a later change? > > Sounds fine to me (maybe include a bug?) skbug.com/4620 https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkBmpStandar... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:264: SkASSERT(this->stream()->hasLength()); On 2015/12/03 19:39:25, scroggo wrote: > I think this belongs above the call to getLength Done. https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:266: SkASSERT(this->stream()->hasPosition()); On 2015/12/03 19:39:24, scroggo wrote: > Same here. Maybe put all the SkASSERTs together? I think that will make it > easier to read. sgtm https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1472933002/diff/130001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:167: * Getter for fSampleX. On 2015/12/03 19:39:25, scroggo wrote: > This line doesn't state anything that cannot be gleaned from looking at the > code. > > I think this should say something about what it means - the swizzler is sampling > every sampleX'th pixel (and discarding the rest). Done.
scroggo@google.com changed reviewers: + djsollen@google.com
On 2015/12/03 19:54:15, msarett wrote: > Derek, can you take a look at the API? Adding Derek. Patch set 6 lgtm
api lgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472933002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472933002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-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 msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1472933002/#ps130002 (title: "Update CodexTest - Ico now supports scanline decoding")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472933002/130002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472933002/130002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472933002/130002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472933002/130002
Message was sent while issue was closed.
Description was changed from ========== Make SkAndroidCodec support ico BUG=skia: ========== to ========== Make SkAndroidCodec support ico BUG=skia: Committed: https://skia.googlesource.com/skia/+/1603e9310f62cf0dd543cdb09dea06aa78373f13 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:130002) as https://skia.googlesource.com/skia/+/1603e9310f62cf0dd543cdb09dea06aa78373f13
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:130002) has been created in https://codereview.chromium.org/1498903004/ by scroggo@google.com. The reason for reverting is: Crashing: https://uberchromegw.corp.google.com/i/client.skia.android/builders/Test-Andr... Also, related ASAN failures: https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G....
Message was sent while issue was closed.
Description was changed from ========== Make SkAndroidCodec support ico BUG=skia: Committed: https://skia.googlesource.com/skia/+/1603e9310f62cf0dd543cdb09dea06aa78373f13 ========== to ========== Make SkAndroidCodec support ico BUG=skia: Committed: https://skia.googlesource.com/skia/+/1603e9310f62cf0dd543cdb09dea06aa78373f13 ==========
Description was changed from ========== Make SkAndroidCodec support ico BUG=skia: Committed: https://skia.googlesource.com/skia/+/1603e9310f62cf0dd543cdb09dea06aa78373f13 ========== to ========== Make SkAndroidCodec support ico BUG=skia: ==========
PTAL, I've fixed the read/write off the edge of memory. https://codereview.chromium.org/1472933002/diff/130002/src/codec/SkBmpStandar... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/130002/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:322: for (int x = startX; x < this->getInfo().width(); x += sampleX) { This style of loop can cause us to run off the edge of memory. We've seen this before, but I forgot about it. Let width = 128 and sampleX = 3. sampledWidth = 128/3 = 42. This loop will iterate on x = 1, 4, 7, ... 127. That is 43 x-values (when the width of memory is only 42). get_dst_coord(127, 3) gives us 42 (which is off the edge of the row). I've fixed this by looping over the sampledWidth and incrementing srcX. This is the strategy we use in SkSwizzler. In the above example, the last value of srcX will be 124.
lgtm at patch set 8. https://codereview.chromium.org/1472933002/diff/180001/src/codec/SkBmpStandar... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/180001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:307: int sampledWidth = get_scaled_dimension(this->getInfo().width(), sampleX); Can you make these variables const? https://codereview.chromium.org/1472933002/diff/180001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:325: for (int x = 0; x < sampledWidth; x++) { Maybe call x dstX?
https://codereview.chromium.org/1472933002/diff/180001/src/codec/SkBmpStandar... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1472933002/diff/180001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:307: int sampledWidth = get_scaled_dimension(this->getInfo().width(), sampleX); On 2015/12/04 15:40:12, scroggo wrote: > Can you make these variables const? Done. https://codereview.chromium.org/1472933002/diff/180001/src/codec/SkBmpStandar... src/codec/SkBmpStandardCodec.cpp:325: for (int x = 0; x < sampledWidth; x++) { On 2015/12/04 15:40:12, scroggo wrote: > Maybe call x dstX? Done.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to the patchset: https://codereview.chromium.org/1472933002/#ps200001 (title: " Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472933002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472933002/200001
Message was sent while issue was closed.
Description was changed from ========== Make SkAndroidCodec support ico BUG=skia: ========== to ========== Make SkAndroidCodec support ico BUG=skia: Committed: https://skia.googlesource.com/skia/+/be8216a922241cc8f3ea3b813608fcb06936fde0 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://skia.googlesource.com/skia/+/be8216a922241cc8f3ea3b813608fcb06936fde0 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
