third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h - Issue 2565323003: Move gif image decoder to SkCodec

Unified Diff: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

Issue 2565323003: Move gif image decoder to SkCodec (Closed)

Patch Set: Setting has alpha on a partial decode. Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « third_party/WebKit/Source/platform/BUILD.gn ('k') | third_party/WebKit/Source/platform/image-decoders/ImageFrame.h » ('j') | third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

index 8ad824902e14b471d8abb796132adcb51c6371c8..8e9de1fefe9c0e8d4390f71b779098b5c1fd06a2 100644

--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

@@ -386,6 +386,8 @@ class PLATFORM_EXPORT ImageDecoder {

// initFrameBuffer() before setting the status to FramePartial. For WebP,

// however, the status needs to be FrameComplete since the complete buffer is

// used to do alpha blending in WEBPImageDecoder::applyPostProcessing().

+ // And gif is also treated differently, since it relies on SkCodec which

scroggo_chromium 2017/04/17 20:04:55 IIUC, previously, GIF and APNG required at least k

cblume 2017/04/20 03:42:26 Your understanding is correct. The way I understan

scroggo_chromium 2017/04/21 20:09:31 It will help if you take a look at the two callsit

It will help if you take a look at the two callsites, in ClearCacheExceptFrame. If the client wants to clear all frames except X, we don't quite honor it, with the thinking that the client will probably want to decode X+1 in the future. If this method returns false for X, we'll *also* keep around the frame that X depends on. The default is to compare the status against empty. If X is empty, clearly keeping it in the cache will not help us decode X+1, so we hang on to the frame that it depended on. Partial is enough to decode any frame that depends on X, since we've already copied the frame that X depends on in order to decode X. I suspect you really want to make sure we're at least in the FramePartial Status. If we've only allocated, then we haven't yet done the copy, so we need to hang on to the frame X depends on in order to finish decoding X. (I just looked back at the new Decode method. I've added a comment there, too, but now I think you want to be in at least the FrameAllocated Status. Though you don't change to that state when you copy the required frame, I believe that was your intent. In which case the default implementation of this method should work fine.)

In general, yes, we want to test public interfaces, but you're changing a behavior. How do we know it's for the better?

cblume 2017/04/21 23:47:38 Okay, I think I follow now. So I understood that

On 2017/04/21 20:09:31, scroggo_chromium wrote: > On 2017/04/20 03:42:26, cblume wrote: > > On 2017/04/17 20:04:55, scroggo_chromium wrote: > > > IIUC, previously, GIF and APNG required at least kFramePartial, and WEBP > > > required kFrameComplete. With this change, GIF matches WEBP, and APNG is the > > > only one to use the default. > > > > > > What is different about using SkCodec that requires the frame to be complete > > > now? Or is it because with SkCodec the Status could now be kFrameAlloc? > > > > > > Can you write a test for this? > > > > Your understanding is correct. > > The way I understand it (which might be wrong) is that if I want to decode > frame > > X+1 (which relies on frame X), I will want to keep frame X around so we don't > > decode from the beginning to get it back. That means I need the complete frame > X > > when using SkCodec. > > > > However, the part I might not understand correctly is why gif didn't > previously > > require the full frame. I'm not sure why a partial frame would be sufficient > to > > decode future frames. > > It will help if you take a look at the two callsites, in ClearCacheExceptFrame. > If the client wants to clear all frames except X, we don't quite honor it, with > the thinking that the client will probably want to decode X+1 in the future. If > this method returns false for X, we'll *also* keep around the frame that X > depends on. > > The default is to compare the status against empty. If X is empty, clearly > keeping it in the cache will not help us decode X+1, so we hang on to the frame > that it depended on. > > Partial is enough to decode any frame that depends on X, since we've already > copied the frame that X depends on in order to decode X. > > I suspect you really want to make sure we're at least in the FramePartial > Status. If we've only allocated, then we haven't yet done the copy, so we need > to hang on to the frame X depends on in order to finish decoding X. (I just > looked back at the new Decode method. I've added a comment there, too, but now I > think you want to be in at least the FrameAllocated Status. Though you don't > change to that state when you copy the required frame, I believe that was your > intent. In which case the default implementation of this method should work > fine.) > > > > > I think we actually don't want to test this. It is a protected method. I > believe > > we want to test public interfaces. > > In general, yes, we want to test public interfaces, but you're changing a > behavior. How do we know it's for the better?

Okay, I think I follow now. So I understood that when we clear frames, we might keep the current frame + the frame it depends on. I now understand that is because we may not be done decoding this frame. In my mind, we were done decoding this frame but maybe the disposal method is DisposePrevious. I guess it can be both cases. I just didn't think of the other. Anyway, I also now see how if we are at least partial, we know then we know the frame this depends on is both complete and still in cache. What threw me is gif (the format, not my patch) requires the required frame to be complete. Essentially, we might need to continue decoding frame |index| before it is actually sufficient for successors. It may actually not be sufficient yet. BUT we are promising that it *can* be sufficient (eventually) so long as we are partially decoded. So now that I understand that, 1.) I think I can just keep the old behavior, and 2.) I think this function should be renamed in a separate CL. :)

scroggo_chromium 2017/04/24 15:06:53 I'm not sure that's the case. If this frame is par

On 2017/04/21 23:47:38, cblume wrote: > On 2017/04/21 20:09:31, scroggo_chromium wrote: > > On 2017/04/20 03:42:26, cblume wrote: > > > On 2017/04/17 20:04:55, scroggo_chromium wrote: > > > > IIUC, previously, GIF and APNG required at least kFramePartial, and WEBP > > > > required kFrameComplete. With this change, GIF matches WEBP, and APNG is > the > > > > only one to use the default. > > > > > > > > What is different about using SkCodec that requires the frame to be > complete > > > > now? Or is it because with SkCodec the Status could now be kFrameAlloc? > > > > > > > > Can you write a test for this? > > > > > > Your understanding is correct. > > > The way I understand it (which might be wrong) is that if I want to decode > > frame > > > X+1 (which relies on frame X), I will want to keep frame X around so we > don't > > > decode from the beginning to get it back. That means I need the complete > frame > > X > > > when using SkCodec. > > > > > > However, the part I might not understand correctly is why gif didn't > > previously > > > require the full frame. I'm not sure why a partial frame would be sufficient > > to > > > decode future frames. > > > > It will help if you take a look at the two callsites, in > ClearCacheExceptFrame. > > If the client wants to clear all frames except X, we don't quite honor it, > with > > the thinking that the client will probably want to decode X+1 in the future. > If > > this method returns false for X, we'll *also* keep around the frame that X > > depends on. > > > > The default is to compare the status against empty. If X is empty, clearly > > keeping it in the cache will not help us decode X+1, so we hang on to the > frame > > that it depended on. > > > > Partial is enough to decode any frame that depends on X, since we've already > > copied the frame that X depends on in order to decode X. > > > > I suspect you really want to make sure we're at least in the FramePartial > > Status. If we've only allocated, then we haven't yet done the copy, so we need > > to hang on to the frame X depends on in order to finish decoding X. (I just > > looked back at the new Decode method. I've added a comment there, too, but now > I > > think you want to be in at least the FrameAllocated Status. Though you don't > > change to that state when you copy the required frame, I believe that was your > > intent. In which case the default implementation of this method should work > > fine.) > > > > > > > > I think we actually don't want to test this. It is a protected method. I > > believe > > > we want to test public interfaces. > > > > In general, yes, we want to test public interfaces, but you're changing a > > behavior. How do we know it's for the better? > > Okay, I think I follow now. > > So I understood that when we clear frames, we might keep the current frame + the > frame it depends on. I now understand that is because we may not be done > decoding this frame. In my mind, we were done decoding this frame but maybe the > disposal method is DisposePrevious. I guess it can be both cases. I just didn't > think of the other. > > Anyway, I also now see how if we are at least partial, we know then we know the > frame this depends on is both complete and still in cache.

I'm not sure that's the case. If this frame is partial, and *not* DisposePrevious, what we're saying is that if the following frame depends on this one, then this frame is decoded enough that there is no need to redecode its required frame in order to decode the following one. At least in theory, we could have evicted the frame that this one depends on (so its status is now Empty), and that does not affect the outcome here.

Agreed.

Yeah, I can see the confusion. Maybe it's clearer if we flip the return value and call it something like NeedRequiredFrameToDecodeSuccessors and return status == kFrameEmpty in the default version.

+ // requires the complete required previous frame.

// Before calling this, verify that frame |index| exists by checking that

// |index| is smaller than |m_frameBufferCache|.size().