Bug 93467 – Change ImageSource to be asynchronous.

• Component: Images
• Status: UNCONFIRMED

[Dongseong Hwang]

This change makes the API of ImageSource support asynchronous image decoding. This
is a preparation for asynchronous image decoding. After ImageSource
compelets image decoding, ImageSource sends NativeImagePtr to BitmapImage via
ImageSourceObserver::didDecodeFrameAtIndex().

This patch renames createFrameAtIndex to requestFrameAtIndex, to imply
this method can be asynchrous. If ImageSource has ImageSourceObserver, this
method sends the result via didDecodeFrameAtIndex() either synchronously or
asynchronously. Otherwise, this method returns the result synchronously.
e.g. GraphicsContext3D calls requestFrameAtIndex without ImageSourceObserver.

[Dongseong Hwang]

Created attachment 157190 [details]
Patch

[Hin-Chung Lam]

High level comments first.

I wonder if it is better to make ImageSource a full asynchronous interface. Which is:

class ImageSource {
  public:
    ImageSource(ImageSourceObserver* observer, ...);
 
    void requestFrameAtIndex(size_t index, bool useAsynchronousDecodingIfPossible = false);
};

class ImageSourceObserver {
  public:
    virtual void didDecodeFrameAtIndex(size_t, NativeImagePtr);
};

And then create a wrapper of SynchronousImageSource:

class SynchronousImageSource : public ImageSourceObserver {
  public:
    SynchronousImageSource(...) : m_imageSource(this, ...) {
    }

    virtual void didDecodeFrameAtIndex(size_t index, NativaImagePtr frame) {
      m_frames[index] = frame;
    }

    NativeImagePtr createFrameAtIndex(size_t index) {
      m_imageSource.requestFrameAtIndex(index);
      return m_frames[index];
    }

  private:
    Vector<NativeImagePtr> m_frames;
    ImageSource m_imageSource;
};

This way we make ImageSource fully asynchronous and separate a synchronous interface. We'll also need to move image decoding in frameDurationAtIndex() and orientationAtIndex() to ImageDecoder such that individual decoder can decide how to implement it. The latter can be done separately though.

[Dongseong Hwang]

(In reply to comment #2)
> This way we make ImageSource fully asynchronous and separate a synchronous interface. We'll also need to move image decoding in frameDurationAtIndex() and orientationAtIndex() to ImageDecoder such that individual decoder can decide how to implement it. The latter can be done separately though.

That's good idea. GraphicsContext3D and others will use SynchronousImageSource instead of ImageSource. I'll update it.

[Dongseong Hwang]

Created attachment 157406 [details]
Patch

[Dongseong Hwang]

(In reply to comment #4)
> Created an attachment (id=157406) [details]
> Patch

This patch introduced SynchronousImageSource, which Alpha suggested.
SynchronousImageSource makes us avoid making a mistake.

So, the changelog was also changed.
"This change makes the API of ImageSource support asynchronous image decoding. This
is a preparation for asynchronous image decoding. After ImageSource
completes image decoding, ImageSource sends NativeImagePtr to BitmapImage via
ImageSourceObserver::didDecodeFrameAtIndex().

This patch renames createFrameAtIndex to requestFrameAtIndex, to imply
this method can be asynchronous.

This patch introduced SynchronousImageSource, which decodes image synchronously.
The main reason to add this class is to prevent clients from misusing
ImageSource. ImageSource will be able to perform sync and async image decoding,
so it will be hard to use ImageSource correctly. So, after this patch, only
BitmapImage and SynchronousImageSource can create and use ImageSource, and other
clients which need to use ImageSource must use SynchronousImageSource.
Currently, GraphicsContext3D and WebImageSkia use SynchronousImageSource."

[Hin-Chung Lam]

Comment on attachment 157406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157406&action=review

> Source/WebCore/ChangeLog:24
> +        No new tests - existing image tests cover these cases.

I believe image layout tests will verify the correctness of this change, but it's best to list the tests here, e.g.

compositing/images/content-image-change.html
fast/images/jpeg-with-color-profile.html

> Source/WebCore/platform/graphics/BitmapImage.cpp:-132
> -    ASSERT(m_decodedSize == 0 || numFrames > 1);

I think we don't need to change this line in this patch.

> Source/WebCore/platform/graphics/ImageSource.cpp:147
> +    ASSERT(m_observer);

I don't think this is necessary since constructor always have this assigned. If this is 0 it will crash in the following line anyway.

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:307
> +    ASSERT(m_observer);

I don't think ASSERT is needed here.

[James Robinson]

Comment on attachment 157406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157406&action=review

> Source/WebCore/platform/graphics/BitmapImage.h:185
> +    virtual void didDecodeFrameAtIndex(size_t, NativeImagePtr);

please add OVERRIDE annotation when you're overriding a virtual. It goes after the signature but before the ;

> Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:60
> +        SynchronousImageSource decoder(ImageSource::AlphaNotPremultiplied,
>                              ignoreGammaAndColorProfile ? ImageSource::GammaAndColorProfileIgnored : ImageSource::GammaAndColorProfileApplied);

please make the parameter line up with opening ( as it did before the type name change

[James Robinson]

I was just checking for style, I don't have opinions about the code itself.

[Hin-Chung Lam]

Dongsung:

I have looked at the code and looks good to me. James helped to check the coding style. Please fix those nits and I think it'll be ready for review. smfr will probably be the best person to review this. Please add him to cc list after you upload a new patch. Thanks.

[Dongseong Hwang]

Created attachment 157681 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
Thanks for review

> I believe image layout tests will verify the correctness of this change, but it's best to list the tests here, e.g.

Ok. I add the paragraph in the changelog

Test: compositing/images/*
      fast/images/*
      fast/dom/HTMLImageElement/*
      http/tests/images/*
      css3/images/*

> > -    ASSERT(m_decodedSize == 0 || numFrames > 1);
> I think we don't need to change this line in this patch.

Ok.

> > +    ASSERT(m_observer);
> I don't think this is necessary since constructor always have this assigned. If this is 0 it will crash in the following line anyway.
> I don't think ASSERT is needed here.

Ditto.

[Dongseong Hwang]

(In reply to comment #7)
Thanks for review.

> please add OVERRIDE annotation when you're overriding a virtual. It goes after the signature but before the ;

Fixed.

> please make the parameter line up with opening ( as it did before the type name change

Ditto.

[Dongseong Hwang]

(In reply to comment #9)
> I have looked at the code and looks good to me. James helped to check the coding style. Please fix those nits and I think it'll be ready for review. smfr will probably be the best person to review this. Please add him to cc list after you upload a new patch. Thanks.

Thanks for your instruction. I added him. :)

[Dongseong Hwang]

(In reply to comment #10)
> Created an attachment (id=157681) [details]
> Patch

The third patch is changed a lot. I described the details in the change log.

ImageSource::frameHasAlphaAtIndex can trigger image decoding again. BitmapImage always queries alpha without triggering image decoding, while SychronousImageSource always triggers image decoding to get alpha.
Clients can not use ImageSource directly. Clients can decode an image with BitmapImage or SychronousImageSource. We could change three methods of ImageSource this way after adding SychronousImageSource.

[Build Bot]

Comment on attachment 157681 [details]
Patch

Attachment 157681 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13478050

[Kwang Yul Seo]

(In reply to comment #15)
> (From update of attachment 157681 [details])
> Attachment 157681 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13478050

Oops, mac is broken.

[Dongseong Hwang]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 157681 [details] [details])
> > Attachment 157681 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/13478050
> 
> Oops, mac is broken.

Changing the mac build system by hand is too hard.
I'll fix it.

[Dongseong Hwang]

Created attachment 157706 [details]
Patch

[Dongseong Hwang]

(In reply to comment #18)
> Created an attachment (id=157706) [details]
> Patch

Make mac build well.

[Hin-Chung Lam]

Comment on attachment 157706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157706&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:-133
> -    

nit: Don't remove this empty line.

> Source/WebCore/platform/graphics/BitmapImage.cpp:150
> +    m_frames[index].m_isComplete = m_source.frameIsCompleteAtIndex(index, ImageSource::NonRequestDecoding);

I can understand why you add this flag here: you want to prevent this call from doing a full image decoding internally. I suggest we don't worry about this yet. It becomes confusing with details at this level because we want to just complete asynchronous ImageSource for this change.

> Source/WebCore/platform/graphics/ImageSource.cpp:178
>      ImageFrame* buffer = m_decoder->frameBufferAtIndex(index);

I understand this function causes a full decode when didDecodeFrameAtIndex() is called and you want to avoid that with RequestDecodingMode. I think a better way to do this is to delegate the call to ImageDecoder. And then only GIFImageDecoder will do a full frame decode, which is not in the scope of parallel image decoding and keep us out of trouble.

I suggest we don't change this implementation, we should narrow our focus to just making ImageSource asynchronous.

> Source/WebCore/platform/graphics/ImageSource.cpp:203
> +

Same here we should let ImageDecoder answer this call.

> Source/WebCore/platform/graphics/ImageSource.cpp:218
> +    if (mode == RequestDecoding)

Same here we don't need to do this. We should have a separate change to move decoding in this call into ImageDecoder.

> Source/WebCore/platform/graphics/ImageSource.h:151
> +    float frameDurationAtIndex(size_t, RequestDecodingMode = RequestDecoding);

I don't think we need this enum. See below in SynchronousImageSource.

> Source/WebCore/platform/graphics/SynchronousImageSource.cpp:106
> +    return m_imageSource.frameHasAlphaAtIndex(index);

We can write this function as:

m_imageSource.requestFrameAtIndex(index);
return m_imageSource.frameHasAlphaAtIndex(index);

If we're worried about future changes we can write a unit test to make sure behavior is correct. Also give a comment to state calling requestFrameAtIndex() is needed.

> Source/WebCore/platform/graphics/SynchronousImageSource.cpp:111
> +    return m_imageSource.frameIsCompleteAtIndex(index);

Same here:

m_imageSource.requestFrameAtIndex(index);
return m_imageSource.frameIsCompleteAtIndex(index);

[Hin-Chung Lam]

Please limit the scope of this change to just refactoring ImageSource to be asynchronous, i.e. limit the scope to just ImageSource::requestFrameAtIndex(), ImageSourceObserver, SynchronousImageSource and switch usage of ImageSource to SynchronousImageSource.

We should do the refactoring of ImageSource, ImageDecoder::decodeAtIndex in a separate patch and discussed in a different bug. Having it included in this change is confusing for other reviewers. I think your second patch with a few nits would be good for review.

[Build Bot]

Comment on attachment 157706 [details]
Patch

Attachment 157706 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13463811

[Dongseong Hwang]

Created attachment 158682 [details]
Patch

[Dongseong Hwang]

(In reply to comment #21)
> Please limit the scope of this change to just refactoring ImageSource to be asynchronous, i.e. limit the scope to just ImageSource::requestFrameAtIndex(), ImageSourceObserver, SynchronousImageSource and switch usage of ImageSource to SynchronousImageSource.
> 
> We should do the refactoring of ImageSource, ImageDecoder::decodeAtIndex in a separate patch and discussed in a different bug. Having it included in this change is confusing for other reviewers. I think your second patch with a few nits would be good for review.

Thank you for your review. I agree with you. I'll file another bug about ImageDecoder.

[Dongseong Hwang]

Created attachment 158695 [details]
Patch

[Dongseong Hwang]

(In reply to comment #20)
> m_imageSource.requestFrameAtIndex(index);
> return m_imageSource.frameHasAlphaAtIndex(index);

We need the code above because ImageSource::frameHasAlphaAtIndex does not decode the given frame.

> 
> m_imageSource.requestFrameAtIndex(index);
> return m_imageSource.frameIsCompleteAtIndex(index);

However, the code above is not yet needed because ImageSource::frameIsCompleteAtIndex decodes the given frame.
A follow-up patch will change frameIsCompleteAtIndex() not to decode anymore, so the code above will be needed. I'll file another bug.

[Andreas Kling]

Comment on attachment 158695 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.