For an opaque JPEG image that is not completely decoded, we report that it is *not* opaque deliberately, since the scanlines that have not been decoded will be transparent.

scroggo@, Is it only jpeg that has the property of transparent non-decoded content? Can an image be decoding over multiple raster calls? Can we change the non-decoded transparent to something else while remaining efficient?

paint-team, Do we care if we fail to draw the things behind a partially decoded jpeg?

We can always figure out a way to detect that we now have decoded the image, but it seems harder than using image metadata information.

> Do we care if we fail to draw the things behind a partially decoded jpeg?

I think yes. Otherwise I think there will be flickering as it decodes.

> scroggo@, Is it only jpeg that has the property of transparent
> non-decoded content?

No, that is true for all image types.

> Can an image be decoding over multiple raster calls?

Yes (if I understand the question). Each time we're asked to draw an image, we only draw the data that has been received so far. So if we have half the data, we decode that. Then if we receive more data and are asked to draw again, we decode the new data.

> Can we change the non-decoded transparent to something else
> while remaining efficient?

Something else like an opaque color? That would be more efficient, but I don't think we would like the result (in the general case).

The part that we want to keep for efficiency is that we allocate the buffer for the whole image once. But we know what portion we haven't decoded (or at least we could). My first thought was that we could use a clip when drawing to only draw the decoded part (which could now be labelled as opaque if it should be). This would require some extra plumbing and I am not sure how hard it would be.

But I think we can do better than that. The clip is a very simple one - we just want all the rows that have been decoded. The way a caller draws the image is by calling ImageFrame::getSkBitmap (or ImageFrame::bitmap - the two methods share the same signature and implementation). We just need to modify it to return a subset. A subset of an SkBitmap is just another SkBitmap that shares the underlying pixels.

This won't be a trivial change, but I think it's mostly straightforward. We also have optimizations in Skia to draw faster when the image is opaque, so this seems like a potential win all around.

> Furthermore, we cache the image metadata in BitmapImage and never update the cache
> when the decoded pixels arrive. So even on subsequent frames, when we repaint due
> to invalidation, we will still consider the image as transparent.

That surprises me. I think you're referring to BitmapImage::ensureFrameIsCached, which will only call BitmapImage::cacheFrame if it has not already cached it (and cacheFrame, in turn, caches whether or not the frame has alpha). But as I understand it, BitmapImage::dataChanged clears those caches when we get more data, so I would expect the next call to ensureFrameIsCached to update whether the frame has alpha. The final alpha value when the image is completely decoded will be correct. But maybe I misunderstand something?

Thanks for all the info.

Regarding clipping to only draw the decoded portion, I think that would introduce a lot of pain in Blink if we wished to take advantage of it. In particular, right now we can do layout based on the reported image size, without the pixels, and we don't need to change that as the pixels come in. We don't want to change the behavior because layout is way more expensive than any optimization we might gain by having partially opaque images. We would also need to maintain the so-far-decoded size and the expected size and somehow keep track of which one to use where.

So I'm certainly in favor of clipping if Skia can take advantage at raster time, but I would not do it for Blink's sake.

Regarding the behavior of the BitmapImage cache, I too expected the dataChanged call to happen. But in my experiments it didn't seem to get called on decode complete. I presumed that was because the "data" refers to the raw undecoded data, which doesn't change. I'll look into that some more.

> Regarding clipping to only draw the decoded portion, I think that would introduce a lot
> of pain in Blink if we wished to take advantage of it. In particular, right now we can
> do layout based on the reported image size, without the pixels, and we don't need to
> change that as the pixels come in.

I am not familiar with layout, but my guess would be that it does not depend on the size of the actual SkBitmap, but based on the dimensions reported by DeferredImageDecoder::size() or ::frameSizeAtIndex(). If my assumption is correct, changing the size of the SkBitmap would not cause layout problems.

> Regarding the behavior of the BitmapImage cache, I too expected the dataChanged call
> to happen. But in my experiments it didn't seem to get called on decode complete.

Image::setData calls dataChanged, which is how the decoder gets its new data in addition to how BitmapImage knows to clear its caches, so I don't see how the decoder got the complete data otherwise. (There is a piece that I haven't completely wrapped my head around though - BitmapImage calls methods on DeferredImageDecoder to cache its information. Those methods will either query its own ImageDecoder, which it uses for the metadata, but then throws away when all the data has been received, OR it will report from its own cache. I wonder if we're reading from the wrong place or something?)

> I presumed that was because the "data" refers to the raw undecoded data, which
> doesn't change. I'll look into that some more.

As I understand it, the raw undecoded data is appended to a SharedBuffer, and dataChanged is called when new data is received.

Agree re: layout, I don't quite see the what the layout problems referred to would be.  Layout an image requires that its size is known; isSizeAvailable(). The image size is read from the image headers.

> As I understand it, the raw undecoded data is appended to a SharedBuffer, and dataChanged is called when new data is received.

Correct.  When the last part of the raw undecoded data arrives, dataChanged() clears the image frame cache, all ImageObservers (page elements that use the image) are advised that new undecoded data has arrived.

ImageObservers that are in view will paint as a result.  That paint requests the image frame, which 1) decodes (ensureFrameIsCached) the final part of the image, and 2) sets the decoded frame status to complete.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp&sq=package:chromium&type=cs&l=123

When a frame is marked complete, the real alpha of the image is categorically known and is set at that time ...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp&sq=package:chromium&type=cs&l=134

Doesn't seem broken to me.  Clipping the image on draw to decoded height is an interesting idea ... see ImageResource::updateImage() 

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/fetch/ImageResource.cpp&rcl=1457882956&l=331

I put an assert in BitmapImage::frameHasAlphaAtIndex to verify that the cached result on alpha matches the underlying image result:

ASSERT(m_source.frameHasAlphaAtIndex(index) == m_frames[index].m_hasAlpha)

and that assert triggers (or it did the last time I checked).

Maybe this is a threading issue, given that we all agree that it should work. The decode is happening on a raster worker thread, while the BitmapImage assert is being evaluated on the renderer thread.

This reminds me of a comment I made: https://bugs.chromium.org/p/chromium/issues/detail?id=505002#c5

So, roughly: when the background-obscuration flag is computed, enough of the  image has not yet been decoded (because of the wrapping into a SkImage-generator I guess), and when the result comes in (at raster time) it's "too late" (quite possibly it could be picked up "later" though)

It does indeed look like https://bugs.chromium.org/p/chromium/issues/detail?id=505002#c5. But the change in alpha status is never picked up, as far as I can tell. Printing a message every time we see a mismatch reveals that the state is not transient.

@noel, the problem is that there is no path back from the ImageFrameGenerator to the BitmapImage object. BitmapImage::dataChanged seems to only get called when the undecoded data is changed.

To complicate things, the ImageFrameGenerator is on the raster worker thread, so there's a thread safety issue in having it somehow get the outcome of the decode all the way back to where the renderer needs it.

@schenney, yes, BitmapImage::dataChanged will be called when the undecoded data is changed.  That will clear the BitmapImage frame caches, and tell the image Observers that they need to repaint.  The repaint will call

 DeferredImageDecode::createFrameAtIndex()

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp&sq=package:chromium&l=93

for a deferred decode.  There is thread locking there to get the alpha state of the image, but that code looks suspect to me since the createFrameAtIndex() call is on the main thread - the decoder has not processed the new undecoded data yet :)

> To complicate things,

We know the alpha of an image frame only when its decode is complete.  But there is no decode complete callback to the main thread.  Not sure if one is needed, or if CC is watching for that state change (decode complete) and reacts correctly.  Complicated.

> We know the alpha of an image frame only when its decode is complete.  But
> there is no decode complete callback to the main thread.  Not sure if one is
> needed, or if CC is watching for that state change (decode complete) and reacts
> correctly.  Complicated.

On a related note, as far as I can tell, DeferredImageDecoder::frameIsCompleteAtIndex never returns true for a deferred decode. It either calls m_actualDecoder->frameIsCompleteAtIndex or it checks m_isComplete, which also comes from m_actualDecoder->frameIsCompleteAtIndex. So if it ever does return true, it means we're decoding on the main thread.

It seems we all agree that we need to get notification of decode complete back to the renderer thread in cases where the decode happened on a raster worker. Alternatively, we could somehow poll the decoder from the renderer thread. Am I missing something?

Not sure we have agreed on that need :)  Most we have noticed so far is that there is no callback notification for decode complete, and the question then becomes: "Well, should we consider adding a callback notification?"

I'd ask why do that?  But say we did, who handles it (CC? Blink?, not sure), and what would they do with it?  Re-draw the image again?  Would that cause over-drawing?

There's a design doc for deferred image decoding, and maybe it discussed all the related issues?

There are several distinct optimizations based on this bit (and in digging around--lots of room for more once opaqueness can be trusted). It might be useful to find real world examples where the opaqueness tracking is failing and focus on cases individually.

> Not sure we have agreed on that need :)  Most we have noticed so far is that
> there is no callback notification for decode complete, and the question then
> becomes: "Well, should we consider adding a callback notification?"

+1

> There's a design doc for deferred image decoding, and maybe it discussed all
> the related issues?

I don't think so. The only thing I see is:

"""
There are a couple metadata getters that require full image decoding, some huge refactoring is needed to unplug these. For example:

BitmapImage::frameDurationAtIndex
BitmapImage::frameIsCompleteAtIndex
BitmapImage::orientationAtIndex
BitmapImage::frameHasAlphaAtIndex

The key to this refactoring is to identify the intention of the code, to determine if a partial decoding or invalid state is acceptable.
"""

> There are several distinct optimizations based on this bit (and in digging around--lots of room for
> more once opaqueness can be trusted). It might be useful to find real world examples where the
> opaqueness tracking is failing and focus on cases individually.

Sgtm. I still think my idea (in comment #4) of setting the alpha type properly at the beginning (when we can, e.g. JPEG) and returning a subset when the SkBitmap is requested would be an interesting option to pursue to give you the right type. It also means we could avoid clearing the SkBitmap at the beginning of decoding. (Should - otherwise we're being dishonest when we claim it's opaque but then clear it to transparent.)

> Sgtm. I still think my idea (in comment #4) of setting the alpha type properly
> at the beginning (when we can, e.g. JPEG) and returning a subset when the
> SkBitmap is requested would be an interesting option to pursue to give you the
> right type.

Nvm. I've realized that this has the same problem with knowing whether the image is complete - the SkImage has a width and height determined at creation time, but we will not decode (and therefore know how many lines have been decoded) until later :(

Yes sadly, you end up in a chicken-and-egg problem since Blink does not know the decoded height.  How could we fix that?

> It also means we could avoid clearing the SkBitmap at the beginning of decoding

Because if we could fix it somehow, we could indeed remove that initial SkBitmap clear (which accounts for about 5% of the decode time cost of an image frame).

Copying relevant comment from https://codereview.chromium.org/1972683002/; proof of concept of combining "header + isFrameCompletellyReceived" information.

On 2016/05/18 20:23:34, Stephen Chennney wrote:
> I think the bug has relevant discussion. We decided not to go with this approach
> because code that used opacity information from the header may not display
> hidden content while the image decodes, which is a less good user experience
> than considering the image to be transparent during loading.

After this I debugged this a bit on Windows to see what is the size of problem.

1) Couldn't find a page that would trigger ImageDecoder::frameHasAlphaAtIndex.

2) DeferredImageDecoder::frameHasAlphaAtIndex gets called, m_actualDecoder is null, and then returns true for multiframe images.
Trace:
  chrome_child.dll!blink::ImageFrameGenerator::hasAlpha(unsigned __int64 index) Line 317 C++
  chrome_child.dll!blink::DeferredImageDecoder::createFrameAtIndex(unsigned __int64 index) Line 119 C++
  chrome_child.dll!blink::ImageSource::createFrameAtIndex(unsigned __int64 index) Line 112 C++
  chrome_child.dll!blink::BitmapImage::decodeAndCacheFrame(unsigned __int64 index) Line 143 C++
  chrome_child.dll!blink::BitmapImage::frameAtIndex(unsigned __int64 index) Line 327 C++
  chrome_child.dll!blink::BitmapImage::currentFrameIsLazyDecoded() Line 389 C++
  chrome_child.dll!blink::GraphicsContext::computeFilterQuality(blink::Image * image, const blink::FloatRect & dest, const blink::FloatRect & src) Line 817 C++

3) Skia optimization.
When SkImage is created in DeferredImageDecoder::createFrameImageAtIndex (same trace as above) opacity info is available only after first decoding. And that decoding happens when there's need to draw bitmap. At least there, in Skia we should be able to use opacity information straight away once the frame is fully decoded. However, SkImageGenerator on creation doesn't have this information (as nothing is yet decoded unless revisiting same part of page) and we dont pass opacity information to Skia through SkImageGenerator::getPixels call. As Leon mentioned, there is Skia optimization when Skia knows that the bitmap is opaque, so it should be possible to do it here. 
Trace:
  chrome_child.dll!blink::GIFImageDecoder::frameComplete(unsigned __int64 frameIndex) Line 189 C++
  chrome_child.dll!GIFImageReader::decode(unsigned __int64 frameIndex) Line 384 C++
  chrome_child.dll!blink::GIFImageDecoder::decode(unsigned __int64 index) Line 312 C++
  chrome_child.dll!blink::ImageDecoder::frameBufferAtIndex(unsigned __int64 index) Line 138 C++
  chrome_child.dll!blink::ImageFrameGenerator::decode(blink::SegmentReader * data, bool allDataReceived, unsigned __int64 index, blink::ImageDecoder * * decoder, SkBitmap * bitmap, SkBitmap::Allocator * allocator) Line 282 C++
  chrome_child.dll!blink::ImageFrameGenerator::tryToResumeDecode(blink::SegmentReader * data, bool allDataReceived, unsigned __int64 index, const SkTSize<int> & scaledSize, SkBitmap::Allocator * allocator) Line 196 C++
  chrome_child.dll!blink::ImageFrameGenerator::decodeAndScale(blink::SegmentReader * data, bool allDataReceived, unsigned __int64 index, const SkImageInfo & info, void * pixels, unsigned __int64 rowBytes) Line 134 C++
  chrome_child.dll!blink::DecodingImageGenerator::onGetPixels(const SkImageInfo & info, void * pixels, unsigned __int64 rowBytes, unsigned int * table, int * tableCount) Line 74 C++
  chrome_child.dll!SkImageGenerator::getPixels(const SkImageInfo & info, void * pixels, unsigned __int64 rowBytes) Line 53 C++
  chrome_child.dll!SkImageCacherator::directGeneratePixels(const SkImageInfo & info, void * pixels, unsigned __int64 rb, int srcX, int srcY) Line 120 C++
  chrome_child.dll!SkImage_Generator::onReadPixels(const SkImageInfo & dstInfo, void * dstPixels, unsigned __int64 dstRB, int srcX, int srcY, SkImage::CachingHint chint) Line 55 C++
  chrome_child.dll!SkImage::readPixels(const SkImageInfo & dstInfo, void * dstPixels, unsigned __int64 dstRowBytes, int srcX, int srcY, SkImage::CachingHint chint) Line 71 C++
  chrome_child.dll!cc::SoftwareImageDecodeController::GetOriginalImageDecode(const cc::ImageDecodeControllerKey & key, sk_sp<SkImage const > image) Line 558 C++
  chrome_child.dll!cc::SoftwareImageDecodeController::DecodeImageInternal(const cc::ImageDecodeControllerKey & key, const cc::DrawImage & draw_image) Line 418 C++
  chrome_child.dll!cc::SoftwareImageDecodeController::DecodeImage(const cc::ImageDecodeControllerKey & key, const cc::DrawImage & image) Line 371 C++
  chrome_child.dll!cc::`anonymous namespace'::ImageDecodeTaskImpl::RunOnWorkerThread() Line 80 C++
  chrome_child.dll!content::RasterWorkerPool::RunTaskInCategoryWithLockAcquired(cc::TaskCategory category) Line 359 C++

#16, #17
I think we need to identify optimizations and see if they are also valid with partial & lazy decode on. I'll debug this further to see what are the optimizations.

https://chromiumcodereview.appspot.com/17999003/diff/17001/Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp

bool DeferredImageDecoder::frameHasAlphaAtIndex(size_t index) const
{
    // FIXME: Report this for multi-frame image.

Apparently this part wasn't originally implemented for multi-frame images.
I enabled it to try (getting alpha from m_frameGenerator->hasAlpha) - it is hit often during animated gif loop.
 

This doesn't have Performance implications according to the speed program definitions. If you disagree, please reach out.