Update comments regarding image caching

Update comments regarding image caching

While attempting to remove the layoutObject parameter the following
observations were made:

 * The decoding forced by BitmapImage::imageForCurrentFrame will be
   deferred so the comment in the header may be removed.

 * BitmapImage::currentFrameKnownToBeOpaque() conservatively returns
   false for uncached images, not true.

 * Since decoding is deferred, there is no guarantee that the
   opaqueness metadata is available after the imageForCurrentFrame
   call has returned. It may increase the chance though, depending on
   image decoder for the particular image.

NOTRY=true
BUG=559131
Committed: https://crrev.com/b36e66be08af9935c7ef51c6bf1fdce45fed61f2
Cr-Commit-Position: refs/heads/master@{#361094}

[davve, 2015-11-20 12:17:54]

Description was changed from

==========
Avoid doing sync decoding for currentFrameKnownToBeOpaque

BUG=
==========

to

==========
[Not for landing]  Avoid doing sync decoding for currentFrameKnownToBeOpaque

BUG=
==========

[davve, 2015-11-20 12:18:04]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 12:18:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1454373005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1454373005/1

[commit-bot: I haz the power, 2015-11-20 13:55:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 13:55:20]

Dry run: This issue passed the CQ dry run.

[davve, 2015-11-20 14:11:10]

Description was changed from

==========
[Not for landing]  Avoid doing sync decoding for currentFrameKnownToBeOpaque

BUG=
==========

to

==========
Avoid calling imageForCurrentFrame when testing opaqueness

It's not a given that imageForCurrentFrame will actually decode any
frame synchronously anyway, so we might as well use the default answer
right away.

This avoids having to pass a layout object (for timeline) down to the
ImageResource.

BUG=559131
==========

[davve, 2015-11-20 14:15:10]

davve@opera.com changed reviewers:
+ fmalita@chromium.org, fs@opera.com

[davve, 2015-11-20 14:15:11]

The imageForFrame call doesn't do what the comment claims anyway: "To get an
accurate answer ...".

In my debug session it happily returns 'false' right there for my very opaque
.jpg when I use it as a background.

[fs, 2015-11-20 14:21:52]

My only qualm with this is that "is opaque"-mechanism seems to be flaky at best
(I noticed this independently), and it would be good if we could make it more
reliable somehow... Maybe file a bug about it (feel free to reference the
bug/comment I mentioned to you "offline"). Otherwise LGTM.

Florin, any differing opinion?

[f(malita), 2015-11-20 14:49:12]

On 2015/11/20 14:15:11, David Vest wrote:
> The imageForFrame call doesn't do what the comment claims anyway: "To get an
> accurate answer ...".
> 
> In my debug session it happily returns 'false' right there for my very opaque
> .jpg when I use it as a background.

The comment is wrong in that the conservative answer is false, not true :)

I think calling imageForCurrentFrame() does help provide a more accurate answer
when the current frame is not yet cached.  It all depends on the timing/ordering
of invocation:

  * if we query BitmapImage::currentFrameKnownToBeOpaque before the frame is
cached, then it will return false, conservatively

  * if we call it after a frame has been cached, then we get back whatever
answer the decoder was able to come up with

I'm not sure whether any of the existing codecs can provide frame opaqueness
answers before decoding, but it's not a stretch to imagine such info could be
stored as frame metadata (same as timing, etc).

If the frame is already cached, imageForCurrentFrame() is a no-op.  If not
cached, it handles caching (but not decoding - all these frames are lazy
decoded), which is work we would have done anyway for painting.

So I think this CL does have a potential to regress under certain conditions
(we'll be treating some otherwise-known-to-be-opaque frames as non-opaque).

Would it be possible to just drop the trace event instead, since we're not
really decoding?  I vaguely recall it is needed to match some other paint
events, but it's worth trying to see if it breaks anything.

[fs, 2015-11-20 15:34:46]

On 2015/11/20 at 14:49:12, fmalita wrote:
> On 2015/11/20 14:15:11, David Vest wrote:
> > The imageForFrame call doesn't do what the comment claims anyway: "To get an
> > accurate answer ...".
> > 
> > In my debug session it happily returns 'false' right there for my very
opaque
> > .jpg when I use it as a background.
> 
> The comment is wrong in that the conservative answer is false, not true :)
> 
> I think calling imageForCurrentFrame() does help provide a more accurate
answer when the current frame is not yet cached.  It all depends on the
timing/ordering of invocation:
> 
>   * if we query BitmapImage::currentFrameKnownToBeOpaque before the frame is
cached, then it will return false, conservatively
> 
>   * if we call it after a frame has been cached, then we get back whatever
answer the decoder was able to come up with

I'm fairly sure I've seen this return "old" results even after decoding finished
(see my last comment in crbug.com/505002), but I guess that could just be a
matter of poor sampling. Anyway, it didn't seem to be reliable...

> I'm not sure whether any of the existing codecs can provide frame opaqueness
answers before decoding, but it's not a stretch to imagine such info could be
stored as frame metadata (same as timing, etc).

I believe the PNG one does, but arguably that's wrong (but who uses opaque PNGs
anyway, right?). In general though, doing so before decoding completes will
yield incorrect results for incrementally displayed images.
 
> If the frame is already cached, imageForCurrentFrame() is a no-op.  If not
cached, it handles caching (but not decoding - all these frames are lazy
decoded), which is work we would have done anyway for painting.
> 
> So I think this CL does have a potential to regress under certain conditions
(we'll be treating some otherwise-known-to-be-opaque frames as non-opaque).
> 
> Would it be possible to just drop the trace event instead, since we're not
really decoding?  I vaguely recall it is needed to match some other paint
events, but it's worth trying to see if it breaks anything.

Maybe fix the comment too then.

[davve, 2015-11-20 16:16:02]

On 2015/11/20 15:34:46, fs wrote:
> On 2015/11/20 at 14:49:12, fmalita wrote:
> > I'm not sure whether any of the existing codecs can provide frame opaqueness
> answers before decoding, but it's not a stretch to imagine such info could be
> stored as frame metadata (same as timing, etc).
> 
> I believe the PNG one does, but arguably that's wrong (but who uses opaque
PNGs
> anyway, right?). In general though, doing so before decoding completes will
> yield incorrect results for incrementally displayed images.

I actually checked PNG (the story seems to be the same for all
formats, but feel free to correct me) and it doesn't seem to handle
this case. When the call to imageForCurrentFrame() comes we have
already thrown away the actualDecoder down in DeferredImageDecoder
(when we went into prepareLazyDecodedFrames mode). We're left with
what the frame generator has, and the frame generator hasn't got any
alpha data before the decoding has happened.

I'm not saying I couldn't work like that though. There might be scenarios
I haven't investigated where the alpha data arrives earlier.

[fs, 2015-11-20 16:33:20]

On 2015/11/20 at 16:16:02, davve wrote:
> On 2015/11/20 15:34:46, fs wrote:
> > On 2015/11/20 at 14:49:12, fmalita wrote:
> > > I'm not sure whether any of the existing codecs can provide frame
opaqueness
> > answers before decoding, but it's not a stretch to imagine such info could
be
> > stored as frame metadata (same as timing, etc).
> > 
> > I believe the PNG one does, but arguably that's wrong (but who uses opaque
PNGs
> > anyway, right?). In general though, doing so before decoding completes will
> > yield incorrect results for incrementally displayed images.
> 
> I actually checked PNG (the story seems to be the same for all
> formats, but feel free to correct me) and it doesn't seem to handle
> this case. When the call to imageForCurrentFrame() comes we have
> already thrown away the actualDecoder down in DeferredImageDecoder
> (when we went into prepareLazyDecodedFrames mode). We're left with
> what the frame generator has, and the frame generator hasn't got any
> alpha data before the decoding has happened.

Just to clarify, what I meant was that the PNG decoder does set alpha-state (to
'false', for RGB images) even during "size-only decodes", so it could be
available early. What you describe above does seem to be similar to what I
observed though...

[davve, 2015-11-20 16:53:38]

On 2015/11/20 16:33:20, fs wrote:

> Just to clarify, what I meant was that the PNG decoder does set alpha-state
(to
> 'false', for RGB images) even during "size-only decodes", so it could be
> available early. What you describe above does seem to be similar to what I
> observed though...

I understand. The encoder handles it, but the information isn't propagated
(yet).

We might want to keep the imageForCurrentFrame call in case the plumbing below
improves?

[fs, 2015-11-20 16:56:09]

On 2015/11/20 at 16:53:38, davve wrote:
> On 2015/11/20 16:33:20, fs wrote:
> 
> > Just to clarify, what I meant was that the PNG decoder does set alpha-state
(to
> > 'false', for RGB images) even during "size-only decodes", so it could be
> > available early. What you describe above does seem to be similar to what I
> > observed though...
> 
> I understand. The encoder handles it, but the information isn't propagated
(yet).
> 
> We might want to keep the imageForCurrentFrame call in case the plumbing below
improves?

It's at least the more conservative way (dropping the LayoutObject arg and the
inspector tracing.)

[f(malita), 2015-11-20 17:06:43]

On 2015/11/20 16:56:09, fs wrote:
> On 2015/11/20 at 16:53:38, davve wrote:
> > On 2015/11/20 16:33:20, fs wrote:
> > 
> > > Just to clarify, what I meant was that the PNG decoder does set
alpha-state
> (to
> > > 'false', for RGB images) even during "size-only decodes", so it could be
> > > available early. What you describe above does seem to be similar to what I
> > > observed though...
> > 
> > I understand. The encoder handles it, but the information isn't propagated
> (yet).
> > 
> > We might want to keep the imageForCurrentFrame call in case the plumbing
below
> improves?
> 
> It's at least the more conservative way (dropping the LayoutObject arg and the
> inspector tracing.)

BTW, I don't want to block this over some hypothetical benefits :)

I'd prefer to only remove the layout object & tracer call if possible, but if
not I'm fine with trying to land it as-is.

[davve, 2015-11-23 07:51:23]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[davve, 2015-11-23 07:51:46]

Description was changed from

==========
Avoid calling imageForCurrentFrame when testing opaqueness

It's not a given that imageForCurrentFrame will actually decode any
frame synchronously anyway, so we might as well use the default answer
right away.

This avoids having to pass a layout object (for timeline) down to the
ImageResource.

BUG=559131
==========

to

==========
Skip trace event for imageForCurrentFrame() call

Since the imageForCurrentFrame() doesn't do synchronous decode anyway,
we might as well skip the timeline trace event.

The underlying motivation is that doing a trace event requires a
layout object, and since fetch/ shouldn't depend on core/, we'd rather
not pass it down.

BUG=559131
==========

[commit-bot: I haz the power, 2015-11-23 07:51:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1454373005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1454373005/40001

[davve, 2015-11-23 07:55:29]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 07:55:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1454373005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1454373005/60001

[commit-bot: I haz the power, 2015-11-23 09:05:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-23 09:05:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[davve, 2015-11-23 09:41:09]

On 2015/11/23 09:05:20, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

virtual/syncpaint/inspector/tracing/decode-resize.html exposed the breakage from
not having the trace.

I suppose the options we have left are:

1. Leave as-is. Let fetch/ still depend on LayoutObject.h.

2. Move imageForCurrentFrame() caching up a level in the
   callstack. This would mean adding imageForCurrentFrame() (or
   something corresponding) to
   StyleFetchedImage{Set,}::knownToBeOpaque and
   LayoutImage::foregroundIsKnownToBeOpaqueInRect.

3. Remove imageForCurrentFrame() call along with trace as in ps #2.

[fs, 2015-11-23 11:15:26]

On 2015/11/23 at 09:41:09, davve wrote:
> On 2015/11/23 09:05:20, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> 
> virtual/syncpaint/inspector/tracing/decode-resize.html exposed the breakage
from not having the trace.
> 
> I suppose the options we have left are:
> 
> 1. Leave as-is. Let fetch/ still depend on LayoutObject.h.
> 
> 2. Move imageForCurrentFrame() caching up a level in the
>    callstack. This would mean adding imageForCurrentFrame() (or
>    something corresponding) to
>    StyleFetchedImage{Set,}::knownToBeOpaque and
>    LayoutImage::foregroundIsKnownToBeOpaqueInRect.
> 
> 3. Remove imageForCurrentFrame() call along with trace as in ps #2.

While I would like to vote for (3), it seems like we might as well go with (1)
to preserve the status quo (even though it seems what's currently measured may
not be very interesting).

[davve, 2015-11-23 13:09:31]

Description was changed from

==========
Skip trace event for imageForCurrentFrame() call

Since the imageForCurrentFrame() doesn't do synchronous decode anyway,
we might as well skip the timeline trace event.

The underlying motivation is that doing a trace event requires a
layout object, and since fetch/ shouldn't depend on core/, we'd rather
not pass it down.

BUG=559131
==========

to

==========
Update comments regarding image caching

While attempting to remove the layoutObject parameter the following
observations were made:

 * The decoding forced by BitmapImage::imageForCurrentFrame will be
   deferred so the comment in the header may be removed.

 * BitmapImage::currentFrameKnownToBeOpaque() conservatively returns
   false for uncached images, not true.

 * Since decoding is deferred, there is no guarantee that the
   opaqueness metadata is available after the imageForCurrentFrame
   call has returned. It may increase the chance though, depending on
   image decoder for the particular image.

NOTRY=true
BUG=559131
==========

[davve, 2015-11-23 13:10:44]

On 2015/11/23 11:15:26, fs wrote:
> On 2015/11/23 at 09:41:09, davve wrote:
> > On 2015/11/23 09:05:20, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > 
> > virtual/syncpaint/inspector/tracing/decode-resize.html exposed the breakage
> from not having the trace.
> > 
> > I suppose the options we have left are:
> > 
> > 1. Leave as-is. Let fetch/ still depend on LayoutObject.h.
> > 
> > 2. Move imageForCurrentFrame() caching up a level in the
> >    callstack. This would mean adding imageForCurrentFrame() (or
> >    something corresponding) to
> >    StyleFetchedImage{Set,}::knownToBeOpaque and
> >    LayoutImage::foregroundIsKnownToBeOpaqueInRect.
> > 
> > 3. Remove imageForCurrentFrame() call along with trace as in ps #2.
> 
> While I would like to vote for (3), it seems like we might as well go with (1)
> to preserve the status quo (even though it seems what's currently measured may
> not be very interesting).

Yes. See ps #5 and updated description.

[fs, 2015-11-23 13:20:08]

On 2015/11/23 at 13:10:44, davve wrote:
> On 2015/11/23 11:15:26, fs wrote:
> > On 2015/11/23 at 09:41:09, davve wrote:
> > > On 2015/11/23 09:05:20, commit-bot: I haz the power wrote:
> > > > Dry run: Try jobs failed on following builders:
> > > >   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > > 
> > > virtual/syncpaint/inspector/tracing/decode-resize.html exposed the
breakage
> > from not having the trace.
> > > 
> > > I suppose the options we have left are:
> > > 
> > > 1. Leave as-is. Let fetch/ still depend on LayoutObject.h.
> > > 
> > > 2. Move imageForCurrentFrame() caching up a level in the
> > >    callstack. This would mean adding imageForCurrentFrame() (or
> > >    something corresponding) to
> > >    StyleFetchedImage{Set,}::knownToBeOpaque and
> > >    LayoutImage::foregroundIsKnownToBeOpaqueInRect.
> > > 
> > > 3. Remove imageForCurrentFrame() call along with trace as in ps #2.
> > 
> > While I would like to vote for (3), it seems like we might as well go with
(1)
> > to preserve the status quo (even though it seems what's currently measured
may
> > not be very interesting).
> 
> Yes. See ps #5 and updated description.

LGTM

[davve, 2015-11-23 13:27:09]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-11-23 13:27:17]

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

[commit-bot: I haz the power, 2015-11-23 13:32:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-23 13:33:08]

Patchset 5 (id:??) landed as
https://crrev.com/b36e66be08af9935c7ef51c6bf1fdce45fed61f2
Cr-Commit-Position: refs/heads/master@{#361094}