Animated GIF imagery with finite looping are falling one loop short
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 16:22:49 +0000 (16:22 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 16:22:49 +0000 (16:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183153

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2018-04-17
Reviewed by Simon Fraser.

Source/WebCore:

The Netscape Looping Application Extension is a block which may be added
to a GIF file to tell the viewer to loop through the entire GIF frames.
This is communicated through two bytes designated for the "loopCount" in
this block.

The entire block may not be found in the GIF, in which case the GIF is
supposed to animate its entire frames only once.

If the block exists and loopCount = 0, this means the image has to loop
through its frames indefinitely.

If the block exist and loopCount > 0, this should mean the image has to
loop through its frames loopCount + 1 times. The extra loop seems to be
the consensus among most of the GIF generators and viewers. For example,
if the image designer wants the image to loop through its frames n times:
-- The GIF generator (e.g. Adobe Photoshop and https://ezgif.com/maker)
   will write n - 1 for loopCount. However http://gifmaker.me and
   http://gifmaker.org write n for loopCount.
-- The browser (e.g. Chrome 65.0.3325 181 and FireFox Quantum 59.0.2) will
   translate loopCount = n - 1 to: animate GIF once + loop n - 1, which
   means loop the GIF n times.

Because the specs are not really clear about this, we are going to consider
the agreed-upon behavior among most of the web browsers the specs here.

* platform/graphics/cg/ImageDecoderCG.cpp:
(WebCore::ImageDecoderCG::repetitionCount const):
* platform/image-decoders/gif/GIFImageDecoder.cpp:
(WebCore::GIFImageDecoder::repetitionCount const):

LayoutTests:

This layout test tests GIF when it has to loop its entire frames a specific
number of times. There are three cases for the loopCount field:
-- loopCount is missing: This means the GIF should animate only once. This
   is covered by animated-red-green-blue-repeat-1.gif.
-- loopCount = 0: This means the image has to animate indefinatly. This
   case is covered by the new GIF animated-red-green-blue-repeat-infinite.gif.
-- loopCount > 0: This will loop the GIF entire frames for (loopCount + 1)
   times. To fix the test with the extra loop, loopCount in
   animated-red-green-blue-repeat-2.gif was changed to 1 instead of 2.

* fast/images/animated-image-loop-count-expected.html:
* fast/images/animated-image-loop-count.html:
* fast/images/resources/animated-red-green-blue-repeat-2.gif:
* fast/images/resources/animated-red-green-blue-repeat-infinite.gif:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230712 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/images/animated-image-loop-count-expected.html
LayoutTests/fast/images/animated-image-loop-count.html
LayoutTests/fast/images/resources/animated-red-green-blue-repeat-2.gif
LayoutTests/fast/images/resources/animated-red-green-blue-repeat-infinite.gif [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp
Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp

index 48bb0d1..ce3154a 100644 (file)
@@ -1,3 +1,25 @@
+2018-04-17  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Animated GIF imagery with finite looping are falling one loop short
+        https://bugs.webkit.org/show_bug.cgi?id=183153
+
+        Reviewed by Simon Fraser.
+
+        This layout test tests GIF when it has to loop its entire frames a specific
+        number of times. There are three cases for the loopCount field:
+        -- loopCount is missing: This means the GIF should animate only once. This
+           is covered by animated-red-green-blue-repeat-1.gif.
+        -- loopCount = 0: This means the image has to animate indefinatly. This
+           case is covered by the new GIF animated-red-green-blue-repeat-infinite.gif.
+        -- loopCount > 0: This will loop the GIF entire frames for (loopCount + 1)
+           times. To fix the test with the extra loop, loopCount in
+           animated-red-green-blue-repeat-2.gif was changed to 1 instead of 2.
+
+        * fast/images/animated-image-loop-count-expected.html:
+        * fast/images/animated-image-loop-count.html:
+        * fast/images/resources/animated-red-green-blue-repeat-2.gif:
+        * fast/images/resources/animated-red-green-blue-repeat-infinite.gif:
+
 2018-04-16  Antoine Quint  <graouts@apple.com>
 
         Layout Test animations/needs-layout.html is a flaky Image Failure.
index 0c11e4c..f0c8f7a 100644 (file)
@@ -9,14 +9,14 @@
 </style>    
 <body>
     <div>
-        <p>Frames of a 3-frame animated image with loopCount = 1:</p>
+        <p>Frames of a 3-frame animated image with missing loopCount, (repetitionCount = 1):</p>
         <div class="box" style="background-color: red;"></div>
         <div class="box" style="background-color: green;"></div>
         <div class="box" style="background-color: blue;"></div>
         <div class="box" style="background-color: blue;"></div>
     </div>
     <div>
-        <p>Frames of a 3-frame animated image with loopCount = 2:</p>
+        <p>Frames of a 3-frame animated image with loopCount = 1, (repetitionCount = 2):</p>
         <div class="box" style="background-color: red;"></div>
         <div class="box" style="background-color: green;"></div>
         <div class="box" style="background-color: blue;"></div>
         <div class="box" style="background-color: blue;"></div>
         <div class="box" style="background-color: blue;"></div>
     </div>
+    <div>
+        <p>Frames of a 3-frame animated image with loopCount = 0, (repetitionCount = infinite):</p>
+        <div class="box" style="background-color: red;"></div>
+        <div class="box" style="background-color: green;"></div>
+        <div class="box" style="background-color: blue;"></div>
+        <div class="box" style="background-color: red;"></div>
+        <div class="box" style="background-color: green;"></div>
+        <div class="box" style="background-color: blue;"></div>
+        <div class="box" style="background-color: red;"></div>
+    </div>
 </body>
 </html>
index 4a5b74e..5090472 100644 (file)
@@ -8,14 +8,14 @@
 </style>    
 <body>
     <div>
-        <p>Frames of a 3-frame animated image with loopCount = 1:</p>
+        <p>Frames of a 3-frame animated image with missing loopCount, (repetitionCount = 1):</p>
         <canvas id="canvas-1"></canvas>
         <canvas id="canvas-2"></canvas>
         <canvas id="canvas-3"></canvas>
         <canvas id="canvas-4"></canvas>
     </div>
     <div>
-        <p>Frames of a 3-frame animated image with loopCount = 2:</p>
+        <p>Frames of a 3-frame animated image with loopCount = 1, (repetitionCount = 2):</p>
         <canvas id="canvas-a"></canvas>
         <canvas id="canvas-b"></canvas>
         <canvas id="canvas-c"></canvas>
         <canvas id="canvas-f"></canvas>
         <canvas id="canvas-g"></canvas>
     </div>
+    <div>
+        <p>Frames of a 3-frame animated image with loopCount = 0, (repetitionCount = infinite):</p>
+        <canvas id="canvas-A"></canvas>
+        <canvas id="canvas-B"></canvas>
+        <canvas id="canvas-C"></canvas>
+        <canvas id="canvas-D"></canvas>
+        <canvas id="canvas-E"></canvas>
+        <canvas id="canvas-F"></canvas>
+        <canvas id="canvas-G"></canvas>
+    </div>
     <script>
         function drawFrame(image, canvasId) {
             return new Promise((resolve) => {
@@ -65,7 +75,8 @@
 
             var images = [
                 { src: "resources/animated-red-green-blue-repeat-1.gif", canvasId: '1', frameCount: 4 },
-                { src: "resources/animated-red-green-blue-repeat-2.gif", canvasId: 'a', frameCount: 7 }
+                { src: "resources/animated-red-green-blue-repeat-2.gif", canvasId: 'a', frameCount: 7 },
+                { src: "resources/animated-red-green-blue-repeat-infinite.gif", canvasId: 'A', frameCount: 7 }
             ];
 
             var promises = [];
index de01138..15b8534 100644 (file)
Binary files a/LayoutTests/fast/images/resources/animated-red-green-blue-repeat-2.gif and b/LayoutTests/fast/images/resources/animated-red-green-blue-repeat-2.gif differ
diff --git a/LayoutTests/fast/images/resources/animated-red-green-blue-repeat-infinite.gif b/LayoutTests/fast/images/resources/animated-red-green-blue-repeat-infinite.gif
new file mode 100644 (file)
index 0000000..50daf08
Binary files /dev/null and b/LayoutTests/fast/images/resources/animated-red-green-blue-repeat-infinite.gif differ
index 4dddd0e..3be85e7 100644 (file)
@@ -1,3 +1,40 @@
+2018-04-17  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Animated GIF imagery with finite looping are falling one loop short
+        https://bugs.webkit.org/show_bug.cgi?id=183153
+
+        Reviewed by Simon Fraser.
+
+        The Netscape Looping Application Extension is a block which may be added
+        to a GIF file to tell the viewer to loop through the entire GIF frames.
+        This is communicated through two bytes designated for the "loopCount" in
+        this block.
+
+        The entire block may not be found in the GIF, in which case the GIF is 
+        supposed to animate its entire frames only once.
+
+        If the block exists and loopCount = 0, this means the image has to loop
+        through its frames indefinitely.
+
+        If the block exist and loopCount > 0, this should mean the image has to
+        loop through its frames loopCount + 1 times. The extra loop seems to be
+        the consensus among most of the GIF generators and viewers. For example,
+        if the image designer wants the image to loop through its frames n times:
+        -- The GIF generator (e.g. Adobe Photoshop and https://ezgif.com/maker) 
+           will write n - 1 for loopCount. However http://gifmaker.me and 
+           http://gifmaker.org write n for loopCount.
+        -- The browser (e.g. Chrome 65.0.3325 181 and FireFox Quantum 59.0.2) will
+           translate loopCount = n - 1 to: animate GIF once + loop n - 1, which 
+           means loop the GIF n times.
+
+        Because the specs are not really clear about this, we are going to consider
+        the agreed-upon behavior among most of the web browsers the specs here.
+
+        * platform/graphics/cg/ImageDecoderCG.cpp:
+        (WebCore::ImageDecoderCG::repetitionCount const):
+        * platform/image-decoders/gif/GIFImageDecoder.cpp:
+        (WebCore::GIFImageDecoder::repetitionCount const):
+
 2018-04-17  Saam Barati  <sbarati@apple.com>
 
         Add system trace points for process launch and for initializeWebProcess
index bd37e73..ed33e7f 100644 (file)
@@ -245,7 +245,9 @@ RepetitionCount ImageDecoderCG::repetitionCount() const
         CFNumberGetValue(num, kCFNumberIntType, &loopCount);
         
         // A property with value 0 means loop forever.
-        return loopCount ? loopCount : RepetitionCountInfinite;
+        // For loopCount > 0, the specs is not clear about it. But it looks the meaning
+        // is: play once + loop loopCount which is equivalent to play loopCount + 1.
+        return loopCount ? loopCount + 1 : RepetitionCountInfinite;
     }
     
     CFDictionaryRef pngProperties = (CFDictionaryRef)CFDictionaryGetValue(properties.get(), kCGImagePropertyPNGDictionary);
index 2b6d1d4..7ba7ed9 100644 (file)
@@ -95,7 +95,7 @@ RepetitionCount GIFImageDecoder::repetitionCount() const
     if (failed() || (m_reader && (!m_reader->imagesCount())))
         m_repetitionCount = RepetitionCountOnce;
     else if (m_reader && m_reader->loopCount() != cLoopCountNotSeen)
-        m_repetitionCount = m_reader->loopCount();
+        m_repetitionCount = m_reader->loopCount() > 0 ? m_reader->loopCount() + 1 : m_reader->loopCount();
     return m_repetitionCount;
 }