Make SkAndroidCodec support gif

Make SkAndroidCodec support gif

Involves a few bug fixes in SkCodec_libgif and a bit more
complexity in SkSwizzler.

BUG=skia:4405
Committed: https://skia.googlesource.com/skia/+/5af4e0bc8fd17944f3c0527462aeba367f6d590a

[msarett, 2015-11-16 23:16:12]

Description was changed from

==========
Make SkAndroidCodec support gif

BUG=skia:4405
==========

to

==========
Make SkAndroidCodec support gif

Involves a few bug fixes in SkCodec_libgif and a bit more
complexity in SkSwizzler.

BUG=skia:4405
==========

[msarett, 2015-11-16 23:20:58]

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp
File dm/DM.cpp (left):

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp#oldcode233
dm/DM.cpp:233: case CodecSrc::kScanline_Subset_Mode:
I'm kind of on the fence about deleting this test.  SkAndroidCodec and BRD have
similar tests.  And it was breaking for gifs...now that I've redesigned, I'm
realizing that I probably could get it to pass for gifs if we want to keep the
test.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cpp
File src/codec/SkCodec_libgif.cpp (left):

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:558: 
We handle this inside the swizzler now.  If we try to handle it in
SkCodec_libgif, then SkCodec_libgif needs to know that we are sampling.

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

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:445: if (fFrameIsSubset) {
This is a just a refactor to share some more code.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:488: for (int y = fFrameRect.top(); y <
fFrameRect.bottom(); y++) {
Bug fix, so outputScanline(y) gives us the right answer.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:557: fFrameRect.top();
Bug fix.

[msarett, 2015-11-16 23:30:17]

msarett@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2015-11-17 14:56:27]

Does this also fix skbug.com/4421

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp
File dm/DM.cpp (left):

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp#oldcode233
dm/DM.cpp:233: case CodecSrc::kScanline_Subset_Mode:
On 2015/11/16 23:20:58, msarett wrote:
> I'm kind of on the fence about deleting this test.  SkAndroidCodec and BRD
have
> similar tests.  And it was breaking for gifs...now that I've redesigned, I'm
> realizing that I probably could get it to pass for gifs if we want to keep the
> test.

It sounds like we have a bug that isn't being caught by other tests?

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

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:445: if (fFrameIsSubset) {
On 2015/11/16 23:20:58, msarett wrote:
> This is a just a refactor to share some more code.

+1

What do you think about creating a pointer locally? i.e.

  SkIRect* frameRect = fFrameIsSubset ? &fFrameRect : nullptr;
  fSwizzler.reset(SkSwizzler::Create(..., frameRect));

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

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:786: int dstWidth = dstInfo.width();
dstWidth = srcWidth?

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:788: // We do not currently support subset decodes for
image types that may have
Add a comment stating this in the header?

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:793: dstWidth = options.fSubset->width();
dstWidth = srcWidth?

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:808: , fSrcOffsetBytes(srcOffset * srcBPP)
Won't srcBPP sometimes be in bits?

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:132: *                 different than if the client
requests a subset decode,
Maybe explain how it's different? Or where an explanation can be found?

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:186: const int           fDstOffset;       // Offset of
the dst in pixels, allows the src to be
a* partial frame... ?

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:191: // for subset decodes.
When you say "accounting for subset decodes", what do you mean? Partial
scanlines? So this is the width of the source subset? Or GIF subset frames? So
this is the width of the frame?

[msarett, 2015-11-17 16:25:19]

Patchset #2 (id:20001) has been deleted

[msarett, 2015-11-17 18:00:50]

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp
File dm/DM.cpp (left):

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp#oldcode233
dm/DM.cpp:233: case CodecSrc::kScanline_Subset_Mode:
On 2015/11/17 14:56:26, scroggo wrote:
> On 2015/11/16 23:20:58, msarett wrote:
> > I'm kind of on the fence about deleting this test.  SkAndroidCodec and BRD
> have
> > similar tests.  And it was breaking for gifs...now that I've redesigned, I'm
> > realizing that I probably could get it to pass for gifs if we want to keep
the
> > test.
> 
> It sounds like we have a bug that isn't being caught by other tests?

Well, the bug is that we don't decode subsets properly for framed gifs.  And I
think we intentionally don't bother to do this.

SkAndroidCodec and BRD subset tests also would catch this, but we don't run
those tests on gifs.

I think there are three options:
(1) Delete the test.
(2) Disable the test for gifs (and might as well for bmps and wbmps as well).
(3) Fix SkSwizzler to support subsets on framed gifs.

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

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:445: if (fFrameIsSubset) {
On 2015/11/17 14:56:26, scroggo wrote:
> On 2015/11/16 23:20:58, msarett wrote:
> > This is a just a refactor to share some more code.
> 
> +1
> 
> What do you think about creating a pointer locally? i.e.
> 
>   SkIRect* frameRect = fFrameIsSubset ? &fFrameRect : nullptr;
>   fSwizzler.reset(SkSwizzler::Create(..., frameRect));

sgtm

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

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:786: int dstWidth = dstInfo.width();
On 2015/11/17 14:56:26, scroggo wrote:
> dstWidth = srcWidth?

Done.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:788: // We do not currently support subset decodes for
image types that may have
On 2015/11/17 14:56:26, scroggo wrote:
> Add a comment stating this in the header?

Done.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:793: dstWidth = options.fSubset->width();
On 2015/11/17 14:56:26, scroggo wrote:
> dstWidth = srcWidth?

Done.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:808: , fSrcOffsetBytes(srcOffset * srcBPP)
On 2015/11/17 14:56:26, scroggo wrote:
> Won't srcBPP sometimes be in bits?

Yes!  I forgot.  I'll find a better name.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:132: *                 different than if the client
requests a subset decode,
On 2015/11/17 14:56:26, scroggo wrote:
> Maybe explain how it's different? Or where an explanation can be found?

Done.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:186: const int           fDstOffset;       // Offset of
the dst in pixels, allows the src to be
On 2015/11/17 14:56:26, scroggo wrote:
> a* partial frame... ?

Acknowledged.

https://codereview.chromium.org/1445313002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:191: // for subset decodes.
On 2015/11/17 14:56:26, scroggo wrote:
> When you say "accounting for subset decodes", what do you mean? Partial
> scanlines? So this is the width of the source subset? Or GIF subset frames? So
> this is the width of the frame?

Acknowledged.

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:188: // Subset Swizzles
This is complicated :(.

I'm continuing to think about how we might simplify things.

[scroggo, 2015-11-17 18:18:25]

lgtm

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp
File dm/DM.cpp (left):

https://codereview.chromium.org/1445313002/diff/1/dm/DM.cpp#oldcode233
dm/DM.cpp:233: case CodecSrc::kScanline_Subset_Mode:
On 2015/11/17 18:00:50, msarett wrote:
> On 2015/11/17 14:56:26, scroggo wrote:
> > On 2015/11/16 23:20:58, msarett wrote:
> > > I'm kind of on the fence about deleting this test.  SkAndroidCodec and BRD
> > have
> > > similar tests.  And it was breaking for gifs...now that I've redesigned,
I'm
> > > realizing that I probably could get it to pass for gifs if we want to keep
> the
> > > test.
> > 
> > It sounds like we have a bug that isn't being caught by other tests?
> 
> Well, the bug is that we don't decode subsets properly for framed gifs.  And I
> think we intentionally don't bother to do this.
> 
> SkAndroidCodec and BRD subset tests also would catch this, but we don't run
> those tests on gifs.
> 
> I think there are three options:
> (1) Delete the test.
> (2) Disable the test for gifs (and might as well for bmps and wbmps as well).
> (3) Fix SkSwizzler to support subsets on framed gifs.

Ah, of course. Yes, I think it makes sense to just delete the test.

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:203: //                          larger than fSrcOffset
(depending on the
Hmm, fSrcOffset is in pixels, but this one is not, so it seems a bit odd to
state that this is "a few pixels larger". I guess you explain that further down
below...

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:228: //         CreateSwizzler in the frame parameter. 
We store subset
I don't think we have a Skia standard for this, but maybe this should be

|frame|

or

"frame"

[msarett, 2015-11-17 18:58:07]

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:203: //                          larger than fSrcOffset
(depending on the
On 2015/11/17 18:18:25, scroggo wrote:
> Hmm, fSrcOffset is in pixels, but this one is not, so it seems a bit odd to
> state that this is "a few pixels larger". I guess you explain that further
down
> below...

Tried to restate this a little clearer.

https://codereview.chromium.org/1445313002/diff/60001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:228: //         CreateSwizzler in the frame parameter. 
We store subset
On 2015/11/17 18:18:25, scroggo wrote:
> I don't think we have a Skia standard for this, but maybe this should be
> 
> |frame|
> 
> or
> 
> "frame"

Done.

[scroggo, 2015-11-17 19:04:55]

LGTM

[msarett, 2015-11-17 19:06:03]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-17 19:06:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445313002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445313002/80001

[commit-bot: I haz the power, 2015-11-17 19:18:07]

Committed patchset #4 (id:80001) as
https://skia.googlesource.com/skia/+/5af4e0bc8fd17944f3c0527462aeba367f6d590a