WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
116041
Checking if frame is complete and access duration doesn't need a decode
https://bugs.webkit.org/show_bug.cgi?id=116041
Summary
Checking if frame is complete and access duration doesn't need a decode
Csaba Osztrogonác
Reported
2013-05-13 09:50:20 PDT
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.
Attachments
GDB backtrace for gifland.us
(18.00 KB, text/plain)
2013-05-14 02:33 PDT
,
Csaba Osztrogonác
no flags
Details
2nd backtrace
(15.63 KB, text/plain)
2013-05-24 07:27 PDT
,
Csaba Osztrogonác
no flags
Details
Patch
(13.34 KB, patch)
2013-06-03 03:50 PDT
,
Csaba Osztrogonác
no flags
Details
Formatted Diff
Diff
with speculative Mac buildfix
(14.57 KB, patch)
2013-06-03 05:02 PDT
,
Csaba Osztrogonác
no flags
Details
Formatted Diff
Diff
proposed patch
(14.54 KB, patch)
2013-06-05 03:47 PDT
,
Csaba Osztrogonác
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
(594.72 KB, application/zip)
2013-06-05 07:37 PDT
,
Build Bot
no flags
Details
Patch
(14.55 KB, patch)
2013-06-11 01:46 PDT
,
Csaba Osztrogonác
no flags
Details
Formatted Diff
Diff
Patch
(16.97 KB, patch)
2013-07-31 01:07 PDT
,
Csaba Osztrogonác
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2013-05-13 10:32:08 PDT
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?
zalan
Comment 2
2013-05-13 11:42:47 PDT
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
Comment 3
2013-05-14 02:33:12 PDT
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
Comment 4
2013-05-14 02:37:16 PDT
It seems this bug is related to Coordinated Graphics somehow.
Noam Rosenthal
Comment 5
2013-05-14 12:43:12 PDT
(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
Comment 6
2013-05-15 03:52:56 PDT
(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
Comment 7
2013-05-15 06:06:00 PDT
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
Comment 8
2013-05-22 00:39:20 PDT
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
Comment 9
2013-05-24 07:26:14 PDT
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
Comment 10
2013-05-24 07:27:10 PDT
Created
attachment 202818
[details]
2nd backtrace
Csaba Osztrogonác
Comment 11
2013-05-27 03:19:47 PDT
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
Comment 12
2013-05-27 13:01:57 PDT
(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
Comment 13
2013-06-03 03:43:11 PDT
(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
Comment 14
2013-06-03 03:50:07 PDT
Created
attachment 203569
[details]
Patch
Build Bot
Comment 15
2013-06-03 04:10:49 PDT
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 16
2013-06-03 04:13:07 PDT
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 17
2013-06-03 04:25:22 PDT
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 18
2013-06-03 04:28:47 PDT
Comment on
attachment 203569
[details]
Patch I try to fix the Mac build.
Csaba Osztrogonác
Comment 19
2013-06-03 05:02:38 PDT
Created
attachment 203577
[details]
with speculative Mac buildfix
Peter Kasting
Comment 20
2013-06-03 12:08:09 PDT
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
Comment 21
2013-06-05 03:47:44 PDT
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 22
2013-06-05 07:36:58 PDT
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
Comment 23
2013-06-05 07:37:01 PDT
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
Comment 24
2013-06-11 01:46:43 PDT
Created
attachment 204328
[details]
Patch I think the EWS reults are unrelated to this bug. Let's try it again.
Csaba Osztrogonác
Comment 25
2013-06-24 06:46:20 PDT
ping for review?
Allan Sandfeld Jensen
Comment 26
2013-06-24 07:23:19 PDT
Comment on
attachment 204328
[details]
Patch LGTM
WebKit Commit Bot
Comment 27
2013-06-25 01:45:10 PDT
Comment on
attachment 204328
[details]
Patch Clearing flags on attachment: 204328 Committed
r151957
: <
http://trac.webkit.org/changeset/151957
>
WebKit Commit Bot
Comment 28
2013-06-25 01:45:15 PDT
All reviewed patches have been landed. Closing bug.
Tobias Netzel
Comment 29
2013-06-25 12:48:02 PDT
***
Bug 93764
has been marked as a duplicate of this bug. ***
WebKit Commit Bot
Comment 30
2013-07-30 09:28:51 PDT
Re-opened since this is blocked by
bug 119267
Csaba Osztrogonác
Comment 31
2013-07-30 09:48:19 PDT
(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
Comment 32
2013-07-30 10:01:31 PDT
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
Comment 33
2013-07-30 10:06:45 PDT
(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
Comment 34
2013-07-30 10:08:34 PDT
(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
Comment 35
2013-07-30 10:09:06 PDT
(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
Comment 36
2013-07-30 12:05:59 PDT
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
Comment 37
2013-07-30 12:24:33 PDT
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
Comment 38
2013-07-30 13:45:43 PDT
(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
Comment 39
2013-07-31 01:07:17 PDT
Created
attachment 207816
[details]
Patch
Csaba Osztrogonác
Comment 40
2013-07-31 01:14:32 PDT
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
Comment 41
2013-07-31 01:17:05 PDT
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)
Comment 42
2013-07-31 01:38:05 PDT
(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
Comment 43
2013-07-31 03:21:38 PDT
(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 44
2013-07-31 06:32:46 PDT
Comment on
attachment 207816
[details]
Patch removing r? until checking better fixes for CG
Csaba Osztrogonác
Comment 45
2013-08-02 09:13:59 PDT
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
Comment 46
2013-10-30 09:16:52 PDT
***
Bug 115846
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug