Re: Save a bitmap copy when advancing to dependent GIF animation frames (issue 2155973002 by aleksandar.stojiljkovic@intel.com)

[aleksandar....@intel.com]

Patch Set 6 : review #34 fixes.
pkasting@, thanks and sorry for the delay on this.



https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode177
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:177:
DCHECK_EQ(frame->bitmap().colorType(), kN32_SkColorType);
On 2016/08/30 06:59:40, Peter Kasting wrote:
> The old code handled this conditionally, so this is a behavior change.
Is this
> safe?
>
> Nit: (expected, actual)

Done. Didn't see how it could be different from kN32 but agree - no need
to introduce behavior change.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
File
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp#newcode98
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:98:
DCHECK(decoder->failed() || imageFrame->getStatus() ==
ImageFrame::FrameComplete);
On 2016/08/30 06:59:40, Peter Kasting wrote:
> This check isn't safe if somehow failed() is false but |imageFrame| is
null.
>
> If that should never happen, I'd change this to:
>
> if (imageFrame && imageFrame->getStatus() ==
ImageFrame::FrameComplete) {
> ...
> } else {
> DCHECK(decoder->failed());
> }

Done. Reverted. As allDataReceived is set to true it is expected that
either it is complete or failed. Didn't see in code how it could be
null.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp#newcode101
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101:
bitmap.setImmutable();
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Why is setting the bitmap as immutable here important? At the very
least this
> needs a comment.
>
> This question basically applies to everywhere in this CL that does
this. I was
> under the impression only people getting an SkImage needed to do this
(to avoid
> a copy), but we seem to be doing it many other places.
>
> If this is because we're passing the bitmap to places that will turn
it into an
> image, can we change to passing an SkImage now?
Reverted this to original implementation - in
ImageSkiaRep::ImageSkiaRep(const SkBitmap& src, float scale) where this
code leads to, bitmap is set to immutable.
[1]
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=1474442403&l=27

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp#newcode101
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101:
bitmap.setImmutable();
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Why is setting the bitmap as immutable here important? At the very
least this
> needs a comment.
>
> This question basically applies to everywhere in this CL that does
this. I was
> under the impression only people getting an SkImage needed to do this
(to avoid
> a copy), but we seem to be doing it many other places.
>
> If this is because we're passing the bitmap to places that will turn
it into an
> image, can we change to passing an SkImage now?
That was my plan too, with all the cases where SkBitmap was ported to
SkImage.
After this patch, only remaining case where we do this is with
WebImage(SkBitmap) construction:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebImage.h?type=cs&q=WebImage&sq=package:chromium&l=94
and there is WebImage::getSkBitmap API that would need to be replaced
with SkImage variant.
The reason I left the SkBitmap/setImmutable used here is that it is
WebKit public/platform API and don't know the deprecation policy here /
if anybody else outside chromium codebase is using it.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp#newcode74
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:74:
*result = frame->bitmap();
Original code seems to be used only by devtools.
After this patch, it should be fine to remove that skia legacy code
under:

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPicture.cpp?rcl=1474471143&l=36

#ifdef SK_SUPPORT_LEGACY_PICTUREINSTALLPIXELREF
class InstallProcImageDeserializer : public SkImageDeserializer {
...

Drawback: now we have unnecessary path here only - toSkSp(fromSkSp()) -
this is the price for not having
fromSkSp(frame->finalizePixelsAndGetImage()) everywhere else.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
File
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp#newcode2148
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2148:
// allow ImageFrame::finalizePixelsAndGetImage to make a copy.
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Isn't it already allowed to make a copy? Does this comment mean
something like
> "We could potentially relax this if we decided it was OK for the
conversion from
> bitmap to image to make a copy in this case."?

By early return if "frame->getStatus() != ImageFrame::FrameComplete" we
ensure that there is no copy done by finalizePixelsAndGetImage. This
means that if don't want to prevent copy by finalizePixelsAndGetImage,
we can relax the check to e.g.:
if (!frame)
return;

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp#newcode2154
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2154:
DCHECK_EQ(bitmap.colorType(), kN32_SkColorType);
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Same comment as in ImageBitmap.cpp.

Done.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode103
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if
(other->m_bitmap.isImmutable() || this == other)
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Tiny efficiency nit: Reverse order of checks

It isn't obvious when/if this could happen (this == other) so while
debugging most common use cases I found that the current order is
leaving the check earlier.

"this == other" I copied from copyBitmapData.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode139
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:139: //
next animation frame by calling takeBitmapDataIfWritable.
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Nit: The header comments should already justify any functional
difference
> between these two, so I'd remove this comment and just inline the
method into
> the header.

Done.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h#newcode103
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:103: //
marked as done (immutable). Returns whether the move succeeded.
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Nit: Maybe better second sentence: "This fails if the provided frame's
bitmap is
> marked immutable, since moving its data out from under it
destructively modifies
> it."

The issue here is not in destructive modification (we don't need that
frame data) but if it is done we cannot write to it when decoding the
next frame.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h#newcode141
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:141:
PassRefPtr<SkImage> finalizePixelsAndGetImage();
On 2016/08/30 06:59:40, Peter Kasting wrote:
> Given that this doesn't always finalize the pixels (if decoding isn't
complete),
> maybe combining these two pieces of functionality isn't a good idea.
It's
> convenient, but it's less clear than I thought it would be since I had
thought
> we were only calling this when the frame was complete, and would thus
finalize
> unconditionally.
>
> It looks like all but one caller knows the frame is complete already.
So we
> could make these do something like:
>
> frame->finalizePixels(); // or ->markBitmapImmutable(), if you
want to be
> more literal
> frame->image();
>
> And that one put the finalizePixels() call under a conditional. Then
it's clear
> when callers are finalizing vs. not.
>
> finalizePixels() could DCHECK that the frame is complete, so people
don't
> accidentally call it for incomplete frames, which would be bad. It
would be
> useful for the places that are currently doing this manually (by
getting the
> bitmap and calling setImmutable() on it) as well, assuming we want to
keep
> those, because it's a bit briefer and clearer than that code (and it
has the
> DCHECK() guard). Finally we could put a comment on the declaration of
> finalizePixels() that this results in saving a copy when image() is
used, but
> disables passing this frame to takeBitmapDataIfWriteable(), so callers
need to
> decide carefully what will result in best performance. (It would be
nice to
> know, in practice, what sorts of sequences of calls can result in
these
> optimization cases overlapping.)
>
> Then we could put a comment on image() that calling it when things are
not
> finalized incurs a copy. If you wanted, this could even be split into
two
> functions, e.g. finalizedImage() and nonFinalizedImage() (or image()
and
> copyImage(), or something), each of which would simply DCHECK whether
or not the
> bitmap was immutable. This would force callers to be explicit about
which one
> they thought they were doing, so as to avoid unintentional copies.

I like finalizePixelsAndGetImage as it explains what it is doing (and as
you said, the only problem is the condition inside). Handled that one
caller separately, updated the comment here and added DCHECK as you
suggested.

https://codereview.chromium.org/2155973002/

[pkas...@chromium.org]

Have not re-reviewed

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
File
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp#newcode101
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101:
bitmap.setImmutable();

On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote:
> The reason I left the SkBitmap/setImmutable used here is that it is
WebKit
> public/platform API and don't know the deprecation policy here / if
anybody else
> outside chromium codebase is using it.

Blink is Chrome's fork, so we assume that we only want things Chrome is
using.

So you should always go ahead and rip out things that you see Chromium
code no longer needing.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode103
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if
(other->m_bitmap.isImmutable() || this == other)

On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote:
> On 2016/08/30 06:59:40, Peter Kasting wrote:
> > Tiny efficiency nit: Reverse order of checks
>
> It isn't obvious when/if this could happen (this == other) so while
debugging
> most common use cases I found that the current order is leaving the
check
> earlier.
>
> "this == other" I copied from copyBitmapData.

I suppose you could make this (and maybe that function also?) DCHECK
that this != other, if it never makes sense for that to happen (which it
probably doesn't).

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
File
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp#newcode101
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101:
bitmap.setImmutable();

On 2016/09/21 21:54:49, Peter Kasting wrote:
> On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote:
> > The reason I left the SkBitmap/setImmutable used here is that it is
WebKit
> > public/platform API and don't know the deprecation policy here / if
anybody
> else
> > outside chromium codebase is using it.
>
> Blink is Chrome's fork, so we assume that we only want things Chrome
is using.
>
> So you should always go ahead and rip out things that you see Chromium
code no
> longer needing.

Checked in details - a lot of layout tests are using WebImage and the
code needs SkBitmap for direct access to pixels - so I left SkBitmap but
there in the WebImage::getSkBitmap API, and seems to be no need to call
setImmutable - on rare places where it is converted to SkImage it is
already happening on client side of the call through [1]
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=1474442403&l=27

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp#newcode74
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:74:
*result = frame->bitmap();

On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote:

> Original code seems to be used only by devtools.
> After this patch, it should be fine to remove that skia legacy code
under:
>
>
https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPicture.cpp?rcl=1474471143&l=36
>
> #ifdef SK_SUPPORT_LEGACY_PICTUREINSTALLPIXELREF
> class InstallProcImageDeserializer : public SkImageDeserializer {
> ...
>
> Drawback: now we have unnecessary path here only - toSkSp(fromSkSp())
- this is
> the price for not having fromSkSp(frame->finalizePixelsAndGetImage())
everywhere
> else.

Done. Removed the drawback.

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode103
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if
(other->m_bitmap.isImmutable() || this == other)

On 2016/09/21 21:54:49, Peter Kasting wrote:
> On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote:
> > On 2016/08/30 06:59:40, Peter Kasting wrote:
> > > Tiny efficiency nit: Reverse order of checks
> >
> > It isn't obvious when/if this could happen (this == other) so while
debugging
> > most common use cases I found that the current order is leaving the
check
> > earlier.
> >
> > "this == other" I copied from copyBitmapData.
>
> I suppose you could make this (and maybe that function also?) DCHECK
that this
> != other, if it never makes sense for that to happen (which it
probably
> doesn't).

Done.

https://codereview.chromium.org/2155973002/

[pkas...@chromium.org]

LGTM


https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode178
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:178: if
(frame->bitmap().colorType() != kN32_SkColorType)
So, what I meant by "is this safe" was not so much "leave this as a
conditional" but simply "can you justify that this does or does not
happen"?

It sounded like you don't think this can ever fail, so maybe we _should_
DCHECK it. An intentional behavior change is OK, I just didn't want an
accidental one.

If we don't DCHECK it, it deserves a comment about when it can fail.

Another question to ask is "do we even care"? Why bother checking the
color type at all? Can't SkImage handle other color types anyway? I
guess I don't really know what the code is trying to do.

Applies in the other place that checks the color type too.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode180
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:180: return
fromSkSp(frame->finalizePixelsAndGetImage());
Nit: Shorter:

return (frame->bitmap().colorType() == kN32_SkColorType) ?
fromSkSp(frame->finalizePixelsAndGetImage()) : nullptr;

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/exported/WebImage.cpp
File third_party/WebKit/Source/platform/exported/WebImage.cpp (right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/exported/WebImage.cpp#newcode84
third_party/WebKit/Source/platform/exported/WebImage.cpp:84: return
WebImage(frame->bitmap());
Nit: Shorter:

return (frame && !decoder->failed()) ? WebImage(frame->bitmap()) :
WebImage();

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/exported/WebImage.cpp#newcode118
third_party/WebKit/Source/platform/exported/WebImage.cpp:118: if
(!bitmap.isNull() && frame->getStatus() == ImageFrame::FrameComplete) {
Nit: No {}

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode122
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:122:
return fromSkSp(SkImage::MakeFromBitmap(frame->bitmap()));
Nit: Avoids duplicating fromSkSp() call:

return fromSkSp((frame->getStatus() == ImageFrame::FrameComplete)
? frame->finalizePixelsAndGetImage()
: SkImage::MakeFromBitmap(frame->bitmap()));

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
(right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp#newcode76
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:76:
return frame->finalizePixelsAndGetImage();
Nit: Shorter:

return (frame && !imageDecoder->failed()) ?
frame->finalizePixelsAndGetImage() : nullptr;

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode136
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:136:
DCHECK(m_status == FrameComplete);
Nit: DCHECK_EQ(FrameComplete, m_status)

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode140
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:140: //
avoid copying in SkImage::MakeFromBitmap.
Nit: Second sentence is already in the header and can be eliminated.
Not really sure what the first sentence is saying. Are you justifying
why setStatus() does not mark the bitmap as immutable? If so, move this
comment there and say something like this instead:

An immutable bitmap would mean callers could construct an SkImage from
this without a copy. However, setting the bitmap immutable here would
defeat takeBitmapDataIfWritable(). Instead we let the bitmap stay
mutable until someone calls finalizePixelsAndGetImage() to actually get
the SkImage.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode141
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:141: if
(!m_bitmap.isImmutable())
Nit: Conditional check unnecessary

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

Patch 9: Rebase.
Path 10: Nits. Thanks pkasting@.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode178
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:178: if
(frame->bitmap().colorType() != kN32_SkColorType)

On 2016/09/22 21:44:08, Peter Kasting wrote:
> So, what I meant by "is this safe" was not so much "leave this as a
conditional"
> but simply "can you justify that this does or does not happen"?
>
> It sounded like you don't think this can ever fail, so maybe we
_should_ DCHECK
> it. An intentional behavior change is OK, I just didn't want an
accidental one.
>
> If we don't DCHECK it, it deserves a comment about when it can fail.
>
> Another question to ask is "do we even care"? Why bother checking the
color
> type at all? Can't SkImage handle other color types anyway? I guess
I don't
> really know what the code is trying to do.
>
> Applies in the other place that checks the color type too.

I didn't see how it could happen with current set of decoders in public
Chromium base. Reverted it back because, after reducing workload I plan
to resume work on Index8 and RGB565 (for Android when target surface is
565) and then to handle this constraints.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode180
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:180: return
fromSkSp(frame->finalizePixelsAndGetImage());

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: Shorter:
>
> return (frame->bitmap().colorType() == kN32_SkColorType) ?
> fromSkSp(frame->finalizePixelsAndGetImage()) : nullptr;

Done.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/exported/WebImage.cpp
File third_party/WebKit/Source/platform/exported/WebImage.cpp (right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/exported/WebImage.cpp#newcode84
third_party/WebKit/Source/platform/exported/WebImage.cpp:84: return
WebImage(frame->bitmap());

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: Shorter:
>
> return (frame && !decoder->failed()) ? WebImage(frame->bitmap()) :
WebImage();

Done.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/exported/WebImage.cpp#newcode118
third_party/WebKit/Source/platform/exported/WebImage.cpp:118: if
(!bitmap.isNull() && frame->getStatus() == ImageFrame::FrameComplete) {

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode122
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:122:
return fromSkSp(SkImage::MakeFromBitmap(frame->bitmap()));

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: Avoids duplicating fromSkSp() call:
>
> return fromSkSp((frame->getStatus() == ImageFrame::FrameComplete)
> ? frame->finalizePixelsAndGetImage()
> : SkImage::MakeFromBitmap(frame->bitmap()));

Done. The most recent refactoring removed the need for fromSkSp
meanwhile.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp
(right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp#newcode76
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:76:
return frame->finalizePixelsAndGetImage();

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: Shorter:
>
> return (frame && !imageDecoder->failed()) ?
> frame->finalizePixelsAndGetImage() : nullptr;

Done.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode136
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:136:
DCHECK(m_status == FrameComplete);

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: DCHECK_EQ(FrameComplete, m_status)

Done.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode140
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:140: //
avoid copying in SkImage::MakeFromBitmap.

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: Second sentence is already in the header and can be eliminated.
Not really
> sure what the first sentence is saying. Are you justifying why
setStatus() does
> not mark the bitmap as immutable? If so, move this comment there and
say
> something like this instead:
>
> An immutable bitmap would mean callers could construct an SkImage from
this
> without a copy. However, setting the bitmap immutable here would
defeat
> takeBitmapDataIfWritable(). Instead we let the bitmap stay mutable
until
> someone calls finalizePixelsAndGetImage() to actually get the SkImage.

Done. Rephrased it, removed the note about copy as it is in the header
and joined it with the comment in setStatus.

https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode141
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:141: if
(!m_bitmap.isImmutable())

On 2016/09/22 21:44:08, Peter Kasting wrote:
> Nit: Conditional check unnecessary

Done.

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

Further owner's review needed.
fmalita@ (changes in platform/graphics and platform/exported)
and dcheng@ (change in core/frame/ImageBitmap.cpp), could you please review?
Thanks,

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

+dcheng@ - reviewer for core/frame/ImageBitmap.cpp change.
Thanks.

https://codereview.chromium.org/2155973002/

[dch...@chromium.org]

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode213
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return

(frame->bitmap().colorType() == kN32_SkColorType) ?

frame->finalizePixelsAndGetImage() : nullptr;
Just curious: what does that mean if we get here and the color type
doesn't match what we expect? Does that mean an error of some sort
occurred earlier in the decoding pipeline?

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode213
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return
(frame->bitmap().colorType() == kN32_SkColorType) ?
frame->finalizePixelsAndGetImage() : nullptr;

On 2016/09/27 22:44:15, dcheng wrote:
> Just curious: what does that mean if we get here and the color type
doesn't
> match what we expect? Does that mean an error of some sort occurred
earlier in
> the decoding pipeline?

kUnknown_SkColorType:
In initial state, SkBitmap has kUnknown_SkColorType. This happens on
error but also when file is partially received, the frame data is not
yet received and the frame not initialized.
In this case ASSERT/DCHECK(!isNull && !isEmpty) is covering the same as
the condition.

some other bitmap type:
Not an error but someone implements a decoder producing different bitmap
type. Don't think there is such case now - I'm working on a patch of
such kind for chromium [1]. Don't know if there are decoders in
different chromium flavors doing it (probably no need to care about it).
[1]
https://codereview.chromium.org/1460523002/


Otherwise, the change in this file is refactoring only. We're not using
isImmutable but it had the same meaning as getStatus() ==
ImageFrame::FrameComplete.

https://codereview.chromium.org/2155973002/

[dch...@chromium.org]

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode213
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return
(frame->bitmap().colorType() == kN32_SkColorType) ?
frame->finalizePixelsAndGetImage() : nullptr;

I understand that this is a refactoring only change, but I don't really
understand it either. Is it possible to add a comment about why we'd
only return an SkImage if the color type matches exactly? How is it
handled if it doesn't match? Do we just not return an image at all? That
doesn't seem completely right either?

(I'm sure this makes more sense to someone who's already familiar with
the decoding pipeline, but I'm not, and so more comments seem like
they'd be useful here)

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

Patch Set 11 : Remove unecessary color check in ImageBitmap. Thanks dcheng@.

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode213
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return
(frame->bitmap().colorType() == kN32_SkColorType) ?
frame->finalizePixelsAndGetImage() : nullptr;

Yes, given the explanation I provided above, there is really no sense in
doing the color check. Removed it.
There are no decoders that produce other color types than N32.
If the frame's bitmap is empty (colortype kUnknown_SkColorType) but
frame marked as complete which cannot happen now, but let's open
possibility for future, it is still perfectly fine to call
SkImage::MakeFromBitmap(m_bitmap) on empty m_bitmap - it returns null.
So, no need to check color type - all cases are handled.

Related to your other concern, returning nullptr from this method is
correct and expected in case of error. All the clients of the call are
handling it.

https://codereview.chromium.org/2155973002/

[dch...@chromium.org]

LGTM

https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

The patch still needs platform/graphics OWNER's review.
CC-ing kbr@ in case fmalita@ is busy/away.
Thanks.

https://codereview.chromium.org/2155973002/

[k...@chromium.org]

A few questions.



https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode107
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:107:
other->m_status = FrameEmpty;
What about all of the other fields of this ImageFrame, like m_status?
Are you sure this ImageFrame object will be in a consistent state after
swapping m_bitmap with other->m_bitmap?

Or, if the invariant is that m_status on both sides must be
FrameComplete for this to run, what about either DCHECKs for that, or
explicit tests and returning false if not?

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
File
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode366
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:366:
if ((buffer->getDisposalMethod() == ImageFrame::DisposeOverwritePrevious
|| !buffer->takeBitmapDataIfWritable(prevBuffer)) &&
!buffer->copyBitmapData(*prevBuffer))
This is a lot of logic for a single if-statement. It's also not clear to
me that if buffer->takeBitmapDataIfWritable() succeeds, that we should
run the logic below, since presumably prevBuffer is already emptied out.
Same comment in the WEBP image decoder.

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode374
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:374:
buffer->zeroFillFrameRect(prevRect);
I don't understand why this preexisting code is calling this against
buffer instead of prevBuffer.

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
File
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode262
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:262:
if ((buffer.getAlphaBlendSource() == ImageFrame::BlendAtopPreviousFrame
|| !buffer.takeBitmapDataIfWritable(&prevBuffer)) &&
!buffer.copyBitmapData(prevBuffer))
Same comment.

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode270
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:270:
buffer.zeroFillFrameRect(prevRect);
Same comment.

https://codereview.chromium.org/2155973002/

[k...@chromium.org]

Also: the description for this CL is very long. Could you put most of the text
(in particular, the patch used for measurement, and maybe more of it) into one
of the associated bugs and link to it instead? Thanks.


https://codereview.chromium.org/2155973002/

[aleksandar....@intel.com]

Patch Set 12 : DCHECK m_status in ImageFrame. Shortened commit message. Thanks
kbr@.

On 2016/09/29 00:41:38, Ken Russell wrote:
> Also: the description for this CL is very long. Could you put most of the text
> (in particular, the patch used for measurement, and maybe more of it) into one
> of the associated bugs and link to it instead? Thanks.

Done.

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
(right):

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode107
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:107:
other->m_status = FrameEmpty;

On 2016/09/29 00:40:04, Ken Russell wrote:
> What about all of the other fields of this ImageFrame, like m_status?
Are you
> sure this ImageFrame object will be in a consistent state after
swapping
> m_bitmap with other->m_bitmap?

For both clients, GIF and WebP right after this call (or copyBitmapData
variant) m_status is set to FramePartial (m_status before this call is
FrameEmpty).

> Or, if the invariant is that m_status on both sides must be
FrameComplete for
> this to run, what about either DCHECKs for that, or explicit tests and
returning
> false if not?

Added DCHECKs: other is always in FrameComplete status, this frame
m_status is FrameEmpty - both of the conditions are ensured on client
side before the call but adding them here too should help clarifying
and avoid misuse later. Thanks.

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
File
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode366
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:366:
if ((buffer->getDisposalMethod() == ImageFrame::DisposeOverwritePrevious
|| !buffer->takeBitmapDataIfWritable(prevBuffer)) &&
!buffer->copyBitmapData(*prevBuffer))

On 2016/09/29 00:40:04, Ken Russell wrote:
> This is a lot of logic for a single if-statement.

It is a nit suggested by pkasting@ and it takes less space this way than
with original:

https://codereview.chromium.org/2155973002/diff2/20001:200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp

> It's also not clear to me that
> if buffer->takeBitmapDataIfWritable() succeeds, that we should run the
logic
> below, since presumably prevBuffer is already emptied out. Same
comment in the
> WEBP image decoder.

Regardless of the way the new frame got the data from previous frame
(via take or via copy), if disposal method is DisposeOverwriteBgcolor,
new frame |buffer| decoding starts from zeroing (making it fully
transparent) previous frame subframe. It keeps the pixels outside
subframe it got from previous frame (which again previous frame
potentially gets from a long dependency chain) but it needs to erase
only the subframe before decoding and writing new values to |buffer|.

https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode374
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:374:
buffer->zeroFillFrameRect(prevRect);

On 2016/09/29 00:40:04, Ken Russell wrote:
> I don't understand why this preexisting code is calling this against
buffer
> instead of prevBuffer.

Partly covered in the comment above. At this point decoding advances
from prevBuffer to buffer, prevBuffer's content is copyed/moved to
|buffer| and prev buffer disposal method specifies what to do in
|buffer|.
The comment states "clear the previous frame to transparent" and "frame"
has a meaning of subrectangle/subframe of the full buffer. It can be
rephrased as "clear the previous buffer's frame in the new buffer".
Don't know if I managed to clarify it.

https://codereview.chromium.org/2155973002/

[k...@chromium.org]

Thanks for the updates. LGTM


https://codereview.chromium.org/2155973002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2155973002/220001


https://codereview.chromium.org/2155973002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #12 (id:220001)

https://codereview.chromium.org/2155973002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 12 (id:??) landed as
https://crrev.com/3a38c33c398189620281ad19a0f8e5b7f6ac3e69
Cr-Commit-Position: refs/heads/master@{#422125}

https://codereview.chromium.org/2155973002/