[Webkit-unassigned] [Bug 184418] ImageFrame type used by non-Cocoa image decoder should not be the same as that used by ImageSource

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 9 12:27:13 PDT 2018


https://bugs.webkit.org/show_bug.cgi?id=184418

--- Comment #16 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180409/00911d8b/attachment-0002.html>


More information about the webkit-unassigned mailing list