Scanline decoding for gifs

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index f9fa818a7826e305e5d443f2eec8705d5b194e32..3a315074998556cdfa02ec61573de9e97c89b5d2 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -87,10 +87,10 @@ SkScanlineDecoder* start_scanline_decoder(SkData* encoded, const SkImageInfo& in
     }
     // DM scanline test assume kTopDown scanline ordering.  Other orderings are
     // tested from within SkScaledCodec.
-    if (SkScanlineDecoder::kTopDown_SkScanlineOrder != scanlineDecoder->getScanlineOrder()) {
+    if (SkCodec::kSuccess != scanlineDecoder->start(info, NULL, colorPtr, colorCountPtr)) {

[scroggo, 2015/08/27 16:35:49]

Why did this move? Also, it seems like the comment applies to the condition
below this?

[msarett, 2015/09/01 17:50:15]

This moved because gifs don't know their SkScanlineOrder until after calling
start().  I moved the comment back into the proper position in the rebase.

         return NULL;
     }
-    if (SkCodec::kSuccess != scanlineDecoder->start(info, NULL, colorPtr, colorCountPtr)) {
+    if (SkScanlineDecoder::kTopDown_SkScanlineOrder != scanlineDecoder->getScanlineOrder()) {

[msarett, 2015/08/24 23:20:12]

In gif we don't know the scanline order until we call onStart().  This is at
least worth documenting, but maybe is bad enough to consider moving more of this
gif logic into ReadHeader?

If we move readUpToFirstImage() and setFrameDims() to ReadHeader(), that will
solve this problem.

[scroggo, 2015/08/27 16:35:49]

We spoke about this a little in person, but I'll add my thoughts here for
posterity.

The downside to moving more information into ReadHeader is that it will take
longer to find out the size of the image. For a lazy client that wants the size
for layout, they'll have to wait longer (not sure how much longer, which should
perhaps also be considered).

This makes me wonder whether it's appropriate that we specify any alpha type
after only reading the header. Or perhaps we should specify
kUnknown_SkAlphaType? There are a couple of good reasons to specify alpha type:
- it allows the client to know whether they can use 565
- it allows the client to ask for the cheaper premul option (i.e. if the client
does not care whether the colors are premultiplied, they should prefer doing
less work)

Okay, I've convinced myself that if we do not move the color table decode into
ReadHeader, we should still report kUnpremul. What we probably really want is be
able to ask the decoder afterwords (or in the call to getPixels?) what the final
alpha type was. We already have that in reallyHasAlpha, although we've been
discussing whether it's necessary (skbug.com/3582), and maybe there is a cleaner
way to do reallyHasAlpha. 

[msarett, 2015/09/01 17:50:15]

Just want to clarify that there are two different issues here:
(1) We don't know if the image has alpha after reading the header.
(2) We don't know SkScanlineOrder after reading the header.

Also, my discussion of what we need to move into ReadHeader() from above is
wrong.  Currently, ReadHeader() calls a giflib function that does 3 things.
(a) Reads signature.
(b) Reads header info.
(c) Reads global color table if it exists.

For (1), we can find out if there is a transparent index by moving
readUpToFirstImage() into ReadHeader().  readUpToFirstImage() should be
lightweight, it only reads extension blocks.  There can 0 to infinity extension
blocks, but normally there ought to be 0 or 1.

We know that the image is definitely opaque if there is no transparent index. 
However, you could make the argument that even if there is a transparent index,
we should still use reallyHasAlpha to make sure that the transparent index is
actually used.

For (2), we need to move readUpToFirstImage() and DGifGetImageDesc() to
ReadHeader().  DGifGetImageDesc() is lightweight - it reads ten bytes.

I think it's best to decide this in person, let's discuss.

[msarett, 2015/09/01 21:37:52]

This issue isn't critical right now.  We'll keep things as is and revisit
further down the road.

         return NULL;
     }
     return scanlineDecoder.detach();