Incremental decode: only use subset for subsetting

Incremental decode: only use subset for subsetting

In the initial patch, we modified the subset passed to incremental
decoding to account for sampling in Y. This allowed SkSampledCodec to
use a tighter bounds. But it also means that the implementation cannot
distinguish between lines that are excluded due to subsetting and those
excluded at the beginning and end due to sampling. This becomes
problematic in GIF (crrev.com/2045293002), which may need to fill,
requiring it to reconstruct the actual destination. In truth,
SkGifCodec does not need to support subsets, but cannot distinguish
between the tighter bounds and a true subset.

Fix this by passing the scaled subset to incremental decode, without
using the tighter bounds.

Make SkSampler::rowNeeded take the starting coordinate into account.

In SkPngCodec, compute the number of rows needed in the output, and use
that as a signal to stop.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2420843003

Committed: https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09

[scroggo_chromium, 2016-10-14 18:20:12]

Description was changed from

==========
Incremental decode: only use subset for subsetting

In the initial patch, we modified the subset passed to incremental
decoding to account for sampling in Y. This allowed SkSampledCodec to
use a tighter bounds. But it also means that the implementation cannot
distinguish between lines that are excluded due to subsetting and those
excluded at the beginning and end due to sampling. This becomes
problematic in GIF (crrev.com/2045293002), which may need to fill,
requiring it to reconstruct the actual destination. In truth,
SkGifCodec does not need to support subsets, but cannot distinguish
between the tighter bounds and a true subset.

Fix this by passing the scaled subset to incremental decode, without
using the tighter bounds.

Make SkSampler::rowNeeded take the starting coordinate into account.

In SkPngCodec, compute the number of rows needed in the output, and use
that as a signal to stop.
==========

to

==========
Incremental decode: only use subset for subsetting

In the initial patch, we modified the subset passed to incremental
decoding to account for sampling in Y. This allowed SkSampledCodec to
use a tighter bounds. But it also means that the implementation cannot
distinguish between lines that are excluded due to subsetting and those
excluded at the beginning and end due to sampling. This becomes
problematic in GIF (crrev.com/2045293002), which may need to fill,
requiring it to reconstruct the actual destination. In truth,
SkGifCodec does not need to support subsets, but cannot distinguish
between the tighter bounds and a true subset.

Fix this by passing the scaled subset to incremental decode, without
using the tighter bounds.

Make SkSampler::rowNeeded take the starting coordinate into account.

In SkPngCodec, compute the number of rows needed in the output, and use
that as a signal to stop.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2420843003
==========

[scroggo_chromium, 2016-10-14 18:22:07]

scroggo@chromium.org changed reviewers:
+ joostouwerling@google.com, msarett@google.com

[msarett, 2016-10-17 20:58:04]

lgtm

[scroggo, 2016-10-17 21:08:51]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2016-10-17 21:08:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 21:32:45]

Description was changed from

==========
Incremental decode: only use subset for subsetting

In the initial patch, we modified the subset passed to incremental
decoding to account for sampling in Y. This allowed SkSampledCodec to
use a tighter bounds. But it also means that the implementation cannot
distinguish between lines that are excluded due to subsetting and those
excluded at the beginning and end due to sampling. This becomes
problematic in GIF (crrev.com/2045293002), which may need to fill,
requiring it to reconstruct the actual destination. In truth,
SkGifCodec does not need to support subsets, but cannot distinguish
between the tighter bounds and a true subset.

Fix this by passing the scaled subset to incremental decode, without
using the tighter bounds.

Make SkSampler::rowNeeded take the starting coordinate into account.

In SkPngCodec, compute the number of rows needed in the output, and use
that as a signal to stop.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2420843003
==========

to

==========
Incremental decode: only use subset for subsetting

In the initial patch, we modified the subset passed to incremental
decoding to account for sampling in Y. This allowed SkSampledCodec to
use a tighter bounds. But it also means that the implementation cannot
distinguish between lines that are excluded due to subsetting and those
excluded at the beginning and end due to sampling. This becomes
problematic in GIF (crrev.com/2045293002), which may need to fill,
requiring it to reconstruct the actual destination. In truth,
SkGifCodec does not need to support subsets, but cannot distinguish
between the tighter bounds and a true subset.

Fix this by passing the scaled subset to incremental decode, without
using the tighter bounds.

Make SkSampler::rowNeeded take the starting coordinate into account.

In SkPngCodec, compute the number of rows needed in the output, and use
that as a signal to stop.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2420843003

Committed:
https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09
==========

[commit-bot: I haz the power, 2016-10-17 21:32:46]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09

[msarett, 2016-10-17 22:20:02]

On 2016/10/17 21:32:46, commit-bot: I haz the power wrote:
> Committed patchset #1 (id:1) as
> https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09

This has caused many untriaged images in Gold.

[scroggo, 2016-10-18 13:57:50]

On 2016/10/17 22:20:02, msarett wrote:
> On 2016/10/17 21:32:46, commit-bot: I haz the power wrote:
> > Committed patchset #1 (id:1) as
> >
https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09
> 
> This has caused many untriaged images in Gold.

From what I can tell, it looks like the scaled down images have all shifted
down, which leads me to believe that when we sampled, we picked lower-numbered
rows when we sampled. This is probably fine, but it would be nice to understand
why that happened. Investigating...

[scroggo, 2016-10-18 14:33:22]

On 2016/10/18 13:57:50, scroggo wrote:
> On 2016/10/17 22:20:02, msarett wrote:
> > On 2016/10/17 21:32:46, commit-bot: I haz the power wrote:
> > > Committed patchset #1 (id:1) as
> > >
> https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09
> > 
> > This has caused many untriaged images in Gold.
> 
> From what I can tell, it looks like the scaled down images have all shifted
> down, which leads me to believe that when we sampled, we picked lower-numbered
> rows when we sampled. This is probably fine, but it would be nice to
understand
> why that happened. Investigating...

Aha - I think all these images are interlaced, and there is a bug in the
interlaced decoder. See crrev.com/2424353003 for the fix.

[msarett1, 2016-10-18 14:53:11]

On 2016/10/18 14:33:22, scroggo wrote:
> On 2016/10/18 13:57:50, scroggo wrote:
> > On 2016/10/17 22:20:02, msarett wrote:
> > > On 2016/10/17 21:32:46, commit-bot: I haz the power wrote:
> > > > Committed patchset #1 (id:1) as
> > > >
> >
https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09
> > > 
> > > This has caused many untriaged images in Gold.
> > 
> > From what I can tell, it looks like the scaled down images have all shifted
> > down, which leads me to believe that when we sampled, we picked
lower-numbered
> > rows when we sampled. This is probably fine, but it would be nice to
> understand
> > why that happened. Investigating...
> 
> Aha - I think all these images are interlaced, and there is a bug in the
> interlaced decoder. See crrev.com/2424353003 for the fix.

Interlaced change looks good to me.  I don't believe that *all* of the images
are
interlaced, but certainly Gold will tell us.

[scroggo, 2016-10-19 15:40:03]

On 2016/10/18 14:53:11, msarett1 wrote:
> On 2016/10/18 14:33:22, scroggo wrote:
> > On 2016/10/18 13:57:50, scroggo wrote:
> > > On 2016/10/17 22:20:02, msarett wrote:
> > > > On 2016/10/17 21:32:46, commit-bot: I haz the power wrote:
> > > > > Committed patchset #1 (id:1) as
> > > > >
> > >
> https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09
> > > > 
> > > > This has caused many untriaged images in Gold.
> > > 
> > > From what I can tell, it looks like the scaled down images have all
shifted
> > > down, which leads me to believe that when we sampled, we picked
> lower-numbered
> > > rows when we sampled. This is probably fine, but it would be nice to
> > understand
> > > why that happened. Investigating...
> > 
> > Aha - I think all these images are interlaced, and there is a bug in the
> > interlaced decoder. See crrev.com/2424353003 for the fix.
> 
> Interlaced change looks good to me.  I don't believe that *all* of the images
> are
> interlaced, but certainly Gold will tell us.

You are correct. I think the interlaced ones are fixed now, but we still have
many images untriaged (many less though!). Once again, I think the new version
is fine, but I'll see if I can figure out why things changed.

[scroggo, 2016-10-20 14:13:11]

On 2016/10/19 15:40:03, scroggo wrote:
> On 2016/10/18 14:53:11, msarett1 wrote:
> > On 2016/10/18 14:33:22, scroggo wrote:
> > > On 2016/10/18 13:57:50, scroggo wrote:
> > > > On 2016/10/17 22:20:02, msarett wrote:
> > > > > On 2016/10/17 21:32:46, commit-bot: I haz the power wrote:
> > > > > > Committed patchset #1 (id:1) as
> > > > > >
> > > >
> >
https://skia.googlesource.com/skia/+/4f2a88cdf0e72d6cdf8d870c5130f45c70c48e09
> > > > > 
> > > > > This has caused many untriaged images in Gold.
> > > > 
> > > > From what I can tell, it looks like the scaled down images have all
> shifted
> > > > down, which leads me to believe that when we sampled, we picked
> > lower-numbered
> > > > rows when we sampled. This is probably fine, but it would be nice to
> > > understand
> > > > why that happened. Investigating...
> > > 
> > > Aha - I think all these images are interlaced, and there is a bug in the
> > > interlaced decoder. See crrev.com/2424353003 for the fix.
> > 
> > Interlaced change looks good to me.  I don't believe that *all* of the
images
> > are
> > interlaced, but certainly Gold will tell us.
> 
> You are correct. I think the interlaced ones are fixed now, but we still have
> many images untriaged (many less though!). Once again, I think the new version
> is fine, but I'll see if I can figure out why things changed.

For posterity, the remaining images should be fixed by
https://codereview.chromium.org/2440563002/