ImageFrame type used by non-Cocoa image decoder should not be the same as that used by ImageSource

• Component: Images
• Status: RESOLVED FIXED

[Antti Koivisto]

ScalableImageDecoder uses ImageFrame class which is meant to be an implementation detail of ImageSource. There are bunch of things in the type that are only used by one or the other client. For hackability there should be an unrelated frame type for ScalableImageDecoder.

[Antti Koivisto]

Created attachment 337496 [details]
patch

[Antti Koivisto]

Created attachment 337497 [details]
patch

[EWS Watchlist]

Attachment 337497 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 337499 [details]
patch

[EWS Watchlist]

Attachment 337499 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 337501 [details]
patch

[EWS Watchlist]

Attachment 337501 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 337504 [details]
patch

[EWS Watchlist]

Attachment 337504 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/39283050>

[Said Abou-Hallawa]

There were two types of the ImageFrame. One was named FrameData, which is used by ImageSource. And the other was named ImageFrame, which was used by the non-Cocoa image decoders.

I tried very hard to merge both in one class named ImageFrame; see <http://trac.webkit.org/changeset/206156>. My goal, which I did not have time to do till now, was to have a single cached ImageFrame for all ports and remove the second copy which the non-Cocoa image decoder still keep.

And now, you are making two implementations of ImageFrame again :).

Can you please reconsider this in your patch? There is no real need to have a second copy with non-Cocoa image decoder. They are almost the same expect the one with non-Cocoa image decoder keeps the pixel data and the NativeImage while the one with the ImageSource keeps the NativeImage only.

If you do not have enough time to do that, I would suggest making ScalableImageDecoderFrame a sub-class of ImageFrame. But PLEASE do not have different implementations for the two classes. This basically roll back what I did and make it difficult to remove the double caching issue.

[Antti Koivisto]

> I tried very hard to merge both in one class named ImageFrame; see
> <http://trac.webkit.org/changeset/206156>. My goal, which I did not have
> time to do till now, was to have a single cached ImageFrame for all ports
> and remove the second copy which the non-Cocoa image decoder still keep.
> 
> And now, you are making two implementations of ImageFrame again :).

I don't understand why. The types share almost nothing of any relevance. Even the actual image data is kept in m_backingStore in the decoder case and in m_nativeImage in ImageSource case. 

It makes it very difficult to make any changes to BitmapImage code as some platform specific decoder always breaks. I think the refactoring to merge the types was misguided.

[Antti Koivisto]

If you read the patch you'll see how much simpler both types are than the combined type. There are a whole bunch of members that are only needed in one use case. All platform specific ifdefs disappear.

ImageFrame can then be further simplified by making it non-copyable.

[Said Abou-Hallawa]

(In reply to Antti Koivisto from comment #12)
> > I tried very hard to merge both in one class named ImageFrame; see
> > <http://trac.webkit.org/changeset/206156>. My goal, which I did not have
> > time to do till now, was to have a single cached ImageFrame for all ports
> > and remove the second copy which the non-Cocoa image decoder still keep.
> > 
> > And now, you are making two implementations of ImageFrame again :).
> 
> I don't understand why. The types share almost nothing of any relevance.
> Even the actual image data is kept in m_backingStore in the decoder case and
> in m_nativeImage in ImageSource case. 
> 

When ImageSource asks the non CG decoder for image metadata or the NativeImage, the decoder checks first its cache. If the metadata or the NativeImage is not available, it calculates it only once and cache it in the ImageFrame. Then it sends it to the ImageSource which caches it again in the other ImageFrame. All the members which are surrounded by #if !USE(CG) and #endif are shared and used by the decoder and the ImageSource. This means all these data members are shared and have the same meaning:

    DecodingStatus m_decodingStatus { DecodingStatus::Invalid };
    IntSize m_size;

    NativeImagePtr m_nativeImage;
    SubsamplingLevel m_subsamplingLevel { SubsamplingLevel::Default };
    DecodingOptions m_decodingOptions;

    ImageOrientation m_orientation { DefaultImageOrientation };
    Seconds m_duration;
    bool m_hasAlpha { true };

> It makes it very difficult to make any changes to BitmapImage code as some
> platform specific decoder always breaks.

What are you trying to achieve? What difficulties does this merge prevent you from doing?

> I think the refactoring to merge the types was misguided.

If you explain your goal and how the double caching can be solved in the future this will help me understand the purpose of this patch. I am just afraid that I or somebody else comes in the future may say the same thing about it: "I think the refactoring to split the ImageFrame was misguided."

[Antti Koivisto]

> When ImageSource asks the non CG decoder for image metadata or the
> NativeImage, the decoder checks first its cache. If the metadata or the
> NativeImage is not available, it calculates it only once and cache it in the
> ImageFrame. Then it sends it to the ImageSource which caches it again in the
> other ImageFrame. All the members which are surrounded by #if !USE(CG) and
> #endif are shared and used by the decoder and the ImageSource. This means
> all these data members are shared and have the same meaning:

There are no functional changes in this patch. ImageFrames for ImageSources don't use m_backingSource and other #if !USE(CG) members at all. Those are only used by the ScalableImageDecoder case. 

The patch makes ImageFrame an implementation detail of ImageSource and adds a new type ScalableImageDecoderFrame that is an implementation detail of ScalableImageDecoder. I think this is better than shared type because

1) They don't share that much data and what is shared is mostly trivial (size etc).  Most of the functions are also only needed for one case.

2) Most important things (like the actual image data) are stored differently. This 
requires ifs in code to handle. Having unused fields in both cases is confusing.

3) They define enum classes that are used only in one client (DisposalMethod and Caching)

4) ImageSource and ScalableImageDecoder are on different abstraction levels. One is generic WebCore/platform type used by all platforms while another is part of platform specific decoder implementation (cleaning up #ifs is direct benefit).

5) They could share a common base class or a common struct containing shared fields. Considering how simple both types are I don't think that would be super helpful either.

Also it is not likely that any further additions to ImageFrame would actually be relevant for the Decoder use case, piling on to the unneeded-field problem.

> What are you trying to achieve? What difficulties does this merge prevent
> you from doing?

I'm adding global tracking for image frames so we can manage memory usage for image animations on global basis rather than per-image. Without cleanups this will end up piling on more complexity and technical debt. 

> If you explain your goal and how the double caching can be solved in the
> future this will help me understand the purpose of this patch. I am just
> afraid that I or somebody else comes in the future may say the same thing
> about it: "I think the refactoring to split the ImageFrame was misguided."

Note that this patch only affects small part of your refactoring. Most of it was really useful.

[Said Abou-Hallawa]

(In reply to Antti Koivisto from comment #15)
> > When ImageSource asks the non CG decoder for image metadata or the
> > NativeImage, the decoder checks first its cache. If the metadata or the
> > NativeImage is not available, it calculates it only once and cache it in the
> > ImageFrame. Then it sends it to the ImageSource which caches it again in the
> > other ImageFrame. All the members which are surrounded by #if !USE(CG) and
> > #endif are shared and used by the decoder and the ImageSource. This means
> > all these data members are shared and have the same meaning:
> 
> There are no functional changes in this patch. ImageFrames for ImageSources
> don't use m_backingSource and other #if !USE(CG) members at all. Those are
> only used by the ScalableImageDecoder case. 
> 

I did not merge ImageFrame and FrameData only because I felt the members can be shared and there is no need to duplicate the code. I merged them because I wanted to fix the double caching problem. If you say, we do not care about the double caching problem for the non-CG decoders, this would be a fine answer for me and I would not raise this issue again and your patch would make sense to me in this case.

> The patch makes ImageFrame an implementation detail of ImageSource and adds
> a new type ScalableImageDecoderFrame that is an implementation detail of
> ScalableImageDecoder. I think this is better than shared type because
> 
> 1) They don't share that much data and what is shared is mostly trivial
> (size etc).  Most of the functions are also only needed for one case.
> 

I think they do. But as you mentioned the difference between the two usages of the ImageFrame is the backingStore and the disposal method. If we care about the double caching problem have then splitting the code makes it more difficult. If we do not care, then your approach or using the sub-classing is fine.

> 2) Most important things (like the actual image data) are stored
> differently. This 
> requires ifs in code to handle. Having unused fields in both cases is
> confusing.
> 

You mean the backingStore and the disposal method. Right? 

> 3) They define enum classes that are used only in one client (DisposalMethod
> and Caching)
> 

I think we agreed on the difference between the two usages differ in adding the backingStore and the disposal method for the non-CG decoders. Right?

> 4) ImageSource and ScalableImageDecoder are on different abstraction levels.
> One is generic WebCore/platform type used by all platforms while another is
> part of platform specific decoder implementation (cleaning up #ifs is direct
> benefit).
> 

This is correct. This was a temporary stage and I wanted to remove this bad design by having a single cache owned by the ImageSource and accessed by the non-CG decoders.

> 5) They could share a common base class or a common struct containing shared
> fields. Considering how simple both types are I don't think that would be
> super helpful either.
> 

I explained above the purpose of merging ImageFrame and FrameData was not only because the data is shared but because I wanted to remove the double caching problem. And we are in agreement that all the data members are used by ImageSource and non-CG decoders expect the backingStore and the disposal method.

> Also it is not likely that any further additions to ImageFrame would
> actually be relevant for the Decoder use case, piling on to the
> unneeded-field problem.
> 

I doubt we will have any additions to ImageFrame in the future. And if we do, I think most likely we will be adding a new metadata member which will be calculated by the decoders. This means the non-CG decoders will be calculating it and caching it in the same new data member.

> > What are you trying to achieve? What difficulties does this merge prevent
> > you from doing?
> 
> I'm adding global tracking for image frames so we can manage memory usage
> for image animations on global basis rather than per-image. Without cleanups
> this will end up piling on more complexity and technical debt. 
> 

Sub classing is another solution which will still make removing the double caching solvable. Can you elaborate more what data members you expect to add, if any, to ImageFrame to finish this work?

> > If you explain your goal and how the double caching can be solved in the
> > future this will help me understand the purpose of this patch. I am just
> > afraid that I or somebody else comes in the future may say the same thing
> > about it: "I think the refactoring to split the ImageFrame was misguided."
> 
> Note that this patch only affects small part of your refactoring. Most of it
> was really useful.

I thank you for appreciating the work I did. Your comment about the merge was misguided is valuable as well. I am trying to understand why you think this was the case and to make sure we are in agreement about this part.

[Antti Koivisto]

> I did not merge ImageFrame and FrameData only because I felt the members can
> be shared and there is no need to duplicate the code. I merged them because
> I wanted to fix the double caching problem. If you say, we do not care about
> the double caching problem for the non-CG decoders, this would be a fine
> answer for me and I would not raise this issue again and your patch would
> make sense to me in this case.

I don't quite understand what the "double caching" problem you say your patches solved was (and the patch doesn't mention anything like that). My patch has no functional changes, it does not revert any fixes. 

Or do you mean that the patch was meant to be some sort of step towards solving a problem that was never completed? Maybe it wasn't the correct step?

> > 2) Most important things (like the actual image data) are stored
> > differently. This 
> > requires ifs in code to handle. Having unused fields in both cases is
> > confusing.
> > 
> 
> You mean the backingStore and the disposal method. Right? 

m_nativeImage, m_subsamplingLevel and m_decodingOptions are equally meaningless for the decoder client.

> I think we agreed on the difference between the two usages differ in adding
> the backingStore and the disposal method for the non-CG decoders. Right?

I don't think I have agreed anything like that. See below.

> This is correct. This was a temporary stage and I wanted to remove this bad
> design by having a single cache owned by the ImageSource and accessed by the
> non-CG decoders.

Your refactoring was done 18 months ago. I don't think it is good for the project to leave temporary stages like that.
 
> I explained above the purpose of merging ImageFrame and FrameData was not
> only because the data is shared but because I wanted to remove the double
> caching problem. And we are in agreement that all the data members are used
> by ImageSource and non-CG decoders expect the backingStore and the disposal
> method.

The following members are only used by one client:

ScalableImageDecoder:
std::unique_ptr<ImageBackingStore> m_backingStore;
DisposalMethod m_disposalMethod { DisposalMethod::Unspecified };

ImageSource:
NativeImagePtr m_nativeImage;
SubsamplingLevel m_subsamplingLevel { SubsamplingLevel::Default };
DecodingOptions m_decodingOptions;

There is also a ton of function that are relevant to only one client as well as the enums. See the patch.

> I doubt we will have any additions to ImageFrame in the future. And if we
> do, I think most likely we will be adding a new metadata member which will
> be calculated by the decoders. This means the non-CG decoders will be
> calculating it and caching it in the same new data member.

I'd like to make ImageFrame non-copyable (see the blocked bug) to be able to efficiently and cleanly track them with maps. I'll also probably add a backpointer to ImageScource.

> Sub classing is another solution which will still make removing the double
> caching solvable. Can you elaborate more what data members you expect to
> add, if any, to ImageFrame to finish this work?

I don't know if this is a problem worth solving nor that sharing some random implementation detail between different parts of architecture is really helpful in solving it. I gather it hasn't been solved over last 18 months so I'm doubtful.

This hypothetical future fix is currently making the code less hackable than it should be, not to mention wasting our time with these unproductive discussions.

[Said Abou-Hallawa]

Comment on attachment 337504 [details]
patch

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

> Source/WebCore/platform/image-decoders/ScalableImageDecoderFrame.cpp:2
> + * Copyright (C) 2016 Apple Inc.  All rights reserved.

2018

> Source/WebCore/platform/image-decoders/ScalableImageDecoderFrame.cpp:-113
>  #if !USE(CG)
> -bool ImageFrame::initialize(const ImageBackingStore& backingStore)

Why we still have this #ifdef? This code will not compile for CG. right?

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:389
> +        ScalableImageDecoderFrame::DisposalMethod prevMethod = prevBuffer->disposalMethod();

I think this can also be auto.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:759
> +    ScalableImageDecoderFrame::DisposalMethod prevMethod = prevBuffer->disposalMethod();

I thing this can also be auto.

[Antti Koivisto]

Created attachment 337691 [details]
patch

[EWS Watchlist]

Attachment 337691 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 337693 [details]
patch

[EWS Watchlist]

Attachment 337693 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 337693 [details]
patch

Clearing flags on attachment: 337693

Committed r230522: <https://trac.webkit.org/changeset/230522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.