[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]

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

--- Comment #17 from Antti Koivisto <koivisto at iki.fi> ---
> 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.

-- 
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/b7624292/attachment-0002.html>