Bug 93458

Summary: [Texmap] Render gif animation well.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: noam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 102043    
Attachments:
Description Flags
Patch
none
Not for review : Layout Test : Animated GIF on a compositing layer
none
Patch
none
Patch none

Description Dongseong Hwang 2012-08-08 03:59:41 PDT
GraphicsLayerTextureMapper::setContentsToImage() checks the pointer to the
image, not nativeImagePtr, so Texmap currently draws only the first frame of gif
animations. This patch makes Texmap draw gif animations.
Comment 1 Dongseong Hwang 2012-08-08 04:02:31 PDT
Created attachment 157169 [details]
Patch
Comment 2 Dongseong Hwang 2012-08-08 04:04:40 PDT
Created attachment 157170 [details]
Not for review : Layout Test : Animated GIF on a compositing layer 

I created this test in order to test the animated gif on Texmap.
It is easy for human to check animated gif rendering, but it is hard to make a complete layout test. I think there is no functionality to capture the specific frame of gif animations in the webkit test runner.
So, I submit this test not for review.
Comment 3 Dongseong Hwang 2012-08-08 04:08:31 PDT
(In reply to comment #2)

The attached test is same to this site : http://www.dorothybrowser.com/test/webkitTest/css3/anigifAC.html
Comment 4 Noam Rosenthal 2012-08-08 06:35:08 PDT
Comment on attachment 157169 [details]
Patch

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

Good catch, see comments.

> Source/WebCore/ChangeLog:12
> +        No new tests, hard to write a test case.

Even harder to catch regressions later on... Please find the right test case for this, e.g. changing the frame and then testing for pixels.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:329
> +    NativeImagePtr newNativeImagePtr = image->nativeImageForCurrentFrame();

You need a null check.
Comment 5 Dongseong Hwang 2012-08-10 00:23:22 PDT
 (In reply to comment #4)
> Even harder to catch regressions later on... Please find the right test case for this, e.g. changing the frame and then testing for pixels.

Thanks for review.
How to test it came to my mind after reading Bug 93171.
I think if php sends a partial gif animation file to webkit, webkit will render the end frame of the received file.
I'll update it.
Comment 6 Dongseong Hwang 2012-08-16 16:34:57 PDT
Created attachment 158939 [details]
Patch
Comment 7 Dongseong Hwang 2012-08-16 16:44:49 PDT
(In reply to comment #4)
> Even harder to catch regressions later on... Please find the right test case for this, e.g. changing the frame and then testing for pixels.

Unfortunately, I could not find prominent solution. I think DRT does not draw animated gif. Even if I tried to capture a image using setInterval, DRT always captured the first frame. We need to change DRT earlier than making a test for this patch.

> 
> You need a null check.

Done. I wrote GraphicsLayerTextureMapper::setContentsToImage similar to GraphicsLayerCA::setContentsToImage.
Comment 8 Noam Rosenthal 2012-08-16 16:46:29 PDT
Comment on attachment 158939 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests, hard to write a test case.

Please change this to "could not write a test due to DRT limitation, see (enter bug ID).
Comment 9 Dongseong Hwang 2012-08-16 17:00:06 PDT
Created attachment 158948 [details]
Patch
Comment 10 Dongseong Hwang 2012-08-16 17:00:39 PDT
(In reply to comment #8)
> Please change this to "could not write a test due to DRT limitation, see (enter bug ID).

I changed the changelog as you suggested. :)
Comment 11 WebKit Review Bot 2012-08-16 17:49:21 PDT
Comment on attachment 158948 [details]
Patch

Clearing flags on attachment: 158948

Committed r125834: <http://trac.webkit.org/changeset/125834>
Comment 12 WebKit Review Bot 2012-08-16 17:49:25 PDT
All reviewed patches have been landed.  Closing bug.