Bug 116041 – Checking if frame is complete and access duration doesn't need a decode

• Component: New Bugs
• Status: REOPENED

[Csaba Osztrogonác]

After https://trac.webkit.org/changeset/149435 WebProcess crahes 
on http://gifland.us . At least with MiniBrowser of Qt and Nix.

With r149434 everything works fine, but with r149435 
WebProcess crashes in 5-10 seconds after the start.

The bug is still valid on ToT. (QtWebKit - r150020 , WebKitNix - last merge: r149902)

It needs more investigation.

[Andreas Kling]

Can't reproduce any badness on applemac port with ~5 minutes of clicking around on gifland.us. Do you have a backtrace for the crash?

[alan]

no crash here either even with frame flattening on (though the iframe on the page has fixed dimensions, so shouldn't make any difference)

[Csaba Osztrogonác]

Created attachment 201693 [details]
GDB backtrace for gifland.us

Here is the GDB backtrace for gifland.us with QtWebKit on r150061.

I checked and I got same backtrace on r149435. Additionally I checked r149434 
too and I got same backtrace on gifland.us as mentioned in the original bug.

[Csaba Osztrogonác]

It seems this bug is related to Coordinated Graphics somehow.

[Noam Rosenthal]

(In reply to comment #4)
> It seems this bug is related to Coordinated Graphics somehow.
It actually seems related to image decoding...

[Csaba Osztrogonác]

(In reply to comment #5)
> (In reply to comment #4)
> > It seems this bug is related to Coordinated Graphics somehow.
> It actually seems related to image decoding...

The assertion hits in image decoder ... but it is somehow related to
Coordinated Graphics, because there are no assertion with WebKit1
based QtTestBrowser and no assertion on Mac. And is related to
r149435 (and r149287), because there were no assertion before r149435.

[Csaba Osztrogonác]

I started debugging it, so assign to me.

The root of the problem is that ImageFrame::setSize() is called again 
after the gif is restarted with the frame 0. But it shouldn't ...
(The assertion hit with WebKit1 and WebKit2 too. But in release mode
only WebKit2 crashes. So it seems it is unrelated to Coordinated Graphics.)

[Csaba Osztrogonác]

149435 is innocent, it only increased the incidence of the crash.
And I managed to reproduce the crash with WK1 and WK2 too.

And then I managed to bisect the real culprit:
https://trac.webkit.org/changeset/143936

[Csaba Osztrogonác]

I found some strange thing during debugging this bug.
Let's see this page: http://gifland.us/198988

The crash happens when the animation is restarted, but not always.
Here is an example simplified trace:
...
- GIFImageDecoder::initFrameBuffer(frameIndex=26)
- ImageSource::clear(destroyAll=false, clearBeforeFrame=25)
   - GIFImageDecoder::clearFrameBufferCache() cleared: 1 frame

- GIFImageDecoder::initFrameBuffer(frameIndex=27)
- ImageSource::clear(destroyAll=false, clearBeforeFrame=26)
   - GIFImageDecoder::clearFrameBufferCache() cleared: 1 frame

- GIFImageDecoder::initFrameBuffer(frameIndex=0)
  -->>>> ImageFrame::setSize(): ASSERT(!width() && !height()); - BOOOOM!
- ImageSource::clear(destroyAll=false, clearBeforeFrame=27)
   - GIFImageDecoder::clearFrameBufferCache() cleared: 2 frames

- GIFImageDecoder::initFrameBuffer(frameIndex=1)
  -->>>> ASSERT(prevBuffer->status() == ImageFrame::FrameComplete); - BOOOOM!

Segmentation fault (core dumped) - see the attached backtrace (2nd attachment)
--> It is caused because the empty frame 0 was copied previously.

[Csaba Osztrogonác]

Created attachment 202818 [details]
2nd backtrace

[Csaba Osztrogonác]

cc Peter as the author of https://bugs.webkit.org/show_bug.cgi?id=23743 
and Dave as the reviewer. Maybe you have any idea how is it possible that
the frame 0 is deleted too early.

[Peter Kasting]

(In reply to comment #11)
> cc Peter as the author of https://bugs.webkit.org/show_bug.cgi?id=23743 
> and Dave as the reviewer. Maybe you have any idea how is it possible that
> the frame 0 is deleted too early.

See https://code.google.com/p/chromium/issues/detail?id=240113#c37 and onward.  In short, I bet you're seeing the same problem we are, and the cause is detailed in comment 39 on that bug -- http://trac.webkit.org/changeset/113486 made a bunch of incorrect changes to BitmapImage.cpp.

We were planning to fix and upstream, but I don't know how long that will take.  It probably wouldn't be hard for you to fix; if you do so I can give you an informal review.

[Csaba Osztrogonác]

(In reply to comment #12)
> See https://code.google.com/p/chromium/issues/detail?id=240113#c37 and onward.  In short, I bet you're seeing the same problem we are, and the cause is detailed in comment 39 on that bug -- http://trac.webkit.org/changeset/113486 made a bunch of incorrect changes to BitmapImage.cpp.
> 
> We were planning to fix and upstream, but I don't know how long that will take.  It probably wouldn't be hard for you to fix; if you do so I can give you an informal review.

Thanks, it seems it is a same bug. I checked https://src.chromium.org/viewvc/blink?revision=149883&view=revision and it it fixed the bug for me.

[Csaba Osztrogonác]

Created attachment 203569 [details]
Patch

[Build Bot]

Comment on attachment 203569 [details]
Patch

Attachment 203569 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/694370

[Build Bot]

Comment on attachment 203569 [details]
Patch

Attachment 203569 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/694362

[Build Bot]

Comment on attachment 203569 [details]
Patch

Attachment 203569 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/693579

[Csaba Osztrogonác]

Comment on attachment 203569 [details]
Patch

I try to fix the Mac build.

[Csaba Osztrogonác]

Created attachment 203577 [details]
with speculative Mac buildfix

[Peter Kasting]

Comment on attachment 203577 [details]
with speculative Mac buildfix

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

Somehow I thought there would need to be other precursor patches upstreamed.  In any case, this direct translation seems pretty OK to me.

> Source/WebCore/platform/graphics/ImageSource.cpp:167
> +    const float duration =  m_decoder->frameDurationAtIndex(index) / 1000.0f;

Nit: Extra space

[Csaba Osztrogonác]

Created attachment 203792 [details]
proposed patch

Extra space removed. :) Fortunately other patches wasn't needed. It was the first imagedecoder commit with behavioural change after the fork.

[Build Bot]

Comment on attachment 203792 [details]
proposed patch

Attachment 203792 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/662588

New failing tests:
inspector/geolocation-success.html

[Build Bot]

Created attachment 203848 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Csaba Osztrogonác]

Created attachment 204328 [details]
Patch

I think the EWS reults are unrelated to this bug. Let's try it again.

[Csaba Osztrogonác]

ping for review?

[Allan Sandfeld Jensen]

Comment on attachment 204328 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 204328 [details]
Patch

Clearing flags on attachment: 204328

Committed r151957: <http://trac.webkit.org/changeset/151957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tobias Netzel]

*** Bug 93764 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Re-opened since this is blocked by bug 119267

[Csaba Osztrogonác]

(In reply to comment #30)
> Re-opened since this is blocked by bug 119267

Reverted, because it revealed a bug on Mac I can't fixed ( bug119267 ):
http://trac.webkit.org/changeset/153475

just for a future note: Once if we can fix it properly on a way makes CG and
other platforms happy too, we should integrate r152531 to this patch too. And
we should fix bug119218 before we remove explicit image decoding from
ImageSource::frameIsCompleteAtIndex() and ImageSourceframeDurationAtIndex()

[Peter Kasting]

There's no information on that bug or this about what the "bug you can't fix" actually is.

(Not that I'm planning on working on this, but whoever does will need to know...)

[Csaba Osztrogonác]

(In reply to comment #32)
> There's no information on that bug or this about what the "bug you can't fix" actually is.
Oh, sorry I referenced to wrong bug. Here is the Mac bug with detailed
information: https://bugs.webkit.org/show_bug.cgi?id=119267
 
> (Not that I'm planning on working on this, but whoever does will need to know...)

:) I'm not expect you work on this, but any hint or idea would
be very welcome. I think (at least hope) if we could fix bug119218 
somehow, we could simple commit r151957 and r152531 again.

[Peter Kasting]

(In reply to comment #33)
> (In reply to comment #32)
> > There's no information on that bug or this about what the "bug you can't fix" actually is.
> Oh, sorry I referenced to wrong bug. Here is the Mac bug with detailed
> information: https://bugs.webkit.org/show_bug.cgi?id=119267

That's the same bug you linked to before.  It doesn't have detailed information, unless I'm really really blind.

[Csaba Osztrogonác]

(In reply to comment #33)
> (In reply to comment #32)
> > There's no information on that bug or this about what the "bug you can't fix" actually is.
...
Ah, it isn't my day, I meant https://bugs.webkit.org/show_bug.cgi?id=119016

[Hin-Chung Lam]

If the problem on CG is that it requires a decode to answer frameIsCompleteAtIndex() then can you do the decode in ImageSource:: frameIsCompleteAtIndex() in ImageSourceCG.cpp?

ImageSource::frameIsCompleteAtIndex() is to answer if the frame is fully loaded whether the frame is decoded or not. WebKit's native and Blink can answer this call without decoding. If CG's decoder doesn't have this capability then the fix should be in ImageSourceCG.cpp, no?

[Tobias Netzel]

Maybe you should look at the patch I had posted rather long time ago in bug 93764, which tried to solve this same issue in a quite naive way.
I've got my own builds of Mac WebKit (using CoreGraphics) running with that patch of mine applied since then (and a little bit longer even). I didn't encounter any issues with my approach and it hugely improves performance of animated GIFs in my case, such that sites with lots of animated GIFs can now finally be viewed at all on OS X 10.5 . Note that there were important internal changes made concerning frame caching in the CoreGraphics image decoder after 10.5 and I did never test this against later versions of OS X.

[Csaba Osztrogonác]

(In reply to comment #36)
> If the problem on CG is that it requires a decode to answer frameIsCompleteAtIndex() then can you do the decode in ImageSource:: frameIsCompleteAtIndex() in ImageSourceCG.cpp?
The idea is good, I like it. Let me try it tomorrow.

> ImageSource::frameIsCompleteAtIndex() is to answer if the frame is fully loaded whether the frame is decoded or not. WebKit's native and Blink can answer this call without decoding. If CG's decoder doesn't have this capability then the fix should be in ImageSourceCG.cpp, no?

I absolutely agree, I said exactly the same thing in bug119016,
but I haven't managed to convince Geoffrey about it and r151957
was considered the guilty.

But let me try your suggestion.

[Csaba Osztrogonác]

Created attachment 207816 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 207816 [details]
Patch

It is the same patch to the original: http://trac.webkit.org/changeset/151957
with these additional fixes/changes:
- Integrated http://trac.webkit.org/changeset/152531 to make EFL and Cairo happy.
- Change CG implementation of ImageSource::frameIsCompleteAtIndex() to explicitly decode image
  to be able determine if a frame is completely loaded or not. Previously it worked because
  BitMapImage::frameIsCompleteAtIndex() explicitly triggered image decoding, before calling 
  ImageSourceCG's ImageSource::frameIsCompleteAtIndex().
- Unfortunately I couldn't make ImageSource::frameIsCompleteAtIndex const, 
  because now the CG's implementation calls the non-const createFrameAtIndex().

[Csaba Osztrogonác]

cc Geoffrey, Mark and Tim from Apple side. Could you accept 
this change to make CG and non-CG platforms happy too?

[Mark Rowe (bdash)]

(In reply to comment #40)
> (From update of attachment 207816 [details])
> - Change CG implementation of ImageSource::frameIsCompleteAtIndex() to explicitly decode image
>   to be able determine if a frame is completely loaded or not. Previously it worked because
>   BitMapImage::frameIsCompleteAtIndex() explicitly triggered image decoding, before calling 
>   ImageSourceCG's ImageSource::frameIsCompleteAtIndex().

Based on my brief reading of the patch this would seem to end up allocating many temporary CGImageRef's due to ImageSource::createFrameAtIndex being called much more frequently. For instance, it'll now be called twice for every call of BitmapImage::cacheFrame (once within frameIsCompleteAtIndex where the result is just discarded, and then again within BitmapImage::cacheFrame itself where the result is actually used). I don't think it's desirable to be adding calls to CGImageSourceCreateImageAtIndex where we then just ignore the result.

[Csaba Osztrogonác]

(In reply to comment #42)
> Based on my brief reading of the patch this would seem to end up allocating many temporary CGImageRef's due to ImageSource::createFrameAtIndex being called much more frequently. For instance, it'll now be called twice for every call of BitmapImage::cacheFrame (once within frameIsCompleteAtIndex where the result is just discarded, and then again within BitmapImage::cacheFrame itself where the result is actually used). I don't think it's desirable to be adding calls to CGImageSourceCreateImageAtIndex where we then just ignore the result.

You're right that BitmapImage::cacheFrame would trigger two calls to
CGImageSourceCreateImageAtIndex. Have you got any idea, how can we
avoid it?

My first proposal in the other bug, which would have left CG implementation
untouched - https://bugs.webkit.org/attachment.cgi?id=207713&action=review - 
was rejected, because you don't want pepper the code with ifdefs.

I have one more idea, it might work: What if CG's
ImageSource::frameIsCompleteAtIndex() didn't call createFrameAtIndex
unconditionally? But only if it is necessary, when the return value is false.
And after the createFrameAtIndex it could return with the recalculated return value. I'm going to try it locally a little bit later today.

[Csaba Osztrogonác]

Comment on attachment 207816 [details]
Patch

removing r? until checking better fixes for CG

[Csaba Osztrogonác]

Set unassigned, I can't work on it until back from holiday.
Feel free to pick it up if anybody is interested in this bug.

[Csaba Osztrogonác]

*** Bug 115846 has been marked as a duplicate of this bug. ***