Bug 176118 – Virtualize ImageDecoder

• Component: Images
• Status: RESOLVED FIXED

[Jer Noble]

Virtualize ImageDecoder

[Jer Noble]

Created attachment 320287 [details]
Patch

[Build Bot]

Comment on attachment 320287 [details]
Patch

Attachment 320287 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4489945

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/composited-animated-gif-outside-viewport.html
fast/images/animated-image-loop-count.html
fast/repaint/animation-after-layer-scroll.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-paint-after-animation.html
http/tests/misc/slow-loading-animated-image.html
fast/repaint/no-animation-outside-viewport-subframe.html
fast/images/animated-gif-webkit-transform.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/animated-image-different-dest-size.html
fast/images/animated-gif-iframe-webkit-transform.html

[Build Bot]

Created attachment 320294 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320287 [details]
Patch

Attachment 320287 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4489953

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/composited-animated-gif-outside-viewport.html
fast/images/animated-image-loop-count.html
fast/repaint/animation-after-layer-scroll.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-paint-after-animation.html
http/tests/misc/slow-loading-animated-image.html
fast/repaint/no-animation-outside-viewport-subframe.html
fast/images/animated-gif-webkit-transform.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/animated-image-different-dest-size.html
fast/images/animated-gif-iframe-webkit-transform.html

[Build Bot]

Created attachment 320297 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320287 [details]
Patch

Attachment 320287 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4489988

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/animated-image-loop-count.html
fast/images/slower-animation-than-decoding-image.html
fast/images/animated-gif-scrolling-crash.html
fast/images/animated-gif-paint-after-animation.html
fast/images/reset-image-animation.html
fast/images/animated-gif-zooming.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/animated-image-different-dest-size.html

[Build Bot]

Created attachment 320303 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 320287 [details]
Patch

Attachment 320287 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4490080

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/composited-animated-gif-outside-viewport.html
fast/images/animated-image-loop-count.html
fast/images/animated-gif-window-resizing.html
fast/images/animated-gif-scrolling-crash.html
fast/repaint/animation-after-layer-scroll.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-paint-after-animation.html
fast/images/gif-loop-count.html
http/tests/misc/slow-loading-animated-image.html
fast/images/animated-gif-no-layout.html
fast/images/animated-gif-zooming.html
fast/repaint/no-animation-outside-viewport-subframe.html
fast/images/animated-gif-webkit-transform.html
fast/images/animated-gif-body-outside-viewport.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/animated-image-different-dest-size.html
fast/images/animated-gif-iframe-webkit-transform.html

[Build Bot]

Created attachment 320307 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 320287 [details]
Patch

Attachment 320287 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4491007

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/animated-gif-webkit-transform.html
fast/images/composited-animated-gif-outside-viewport.html
fast/images/animated-image-loop-count.html
fast/repaint/animation-after-layer-scroll.html
fast/images/animated-gif-scrolling-crash.html
fast/images/animated-gif-window-resizing.html
fast/images/animated-gif-iframe-webkit-transform.html
fast/images/animated-gif-paint-after-animation.html
fast/images/gif-loop-count.html
http/tests/misc/slow-loading-animated-image.html
fast/images/animated-gif-no-layout.html
fast/images/animated-gif-zooming.html
fast/repaint/no-animation-outside-viewport-subframe.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-body-outside-viewport.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/animated-image-different-dest-size.html

[Build Bot]

Created attachment 320314 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

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

[Miguel Gomez]

Maybe it would be interesting to fix https://bugs.webkit.org/show_bug.cgi?id=172773 before fixing this. It will define a single ImageDecoder.h for all the ports, and then this change can be performed easier.

[Jer Noble]

(In reply to Miguel Gomez from comment #12)
> Maybe it would be interesting to fix
> https://bugs.webkit.org/show_bug.cgi?id=172773 before fixing this. It will
> define a single ImageDecoder.h for all the ports, and then this change can
> be performed easier.

WRT that patch, I'm not enthusiastic about how many platform details are exposed the virtual base class.  I think that it would be much cleaner to merge Said's patch into this one than vice versa.

[Jer Noble]

Created attachment 320620 [details]
Patch for landing

[Miguel Gomez]

Both GTK and WPE ports already have an ImageDecoder superclass in Source/WebCore/platform/image-decoders. I guess that adding a new ImageDecoder definition in Source/WebCore/platform is what is causing WPE and GTK bots to fail.

[Jer Noble]

(In reply to Miguel Gomez from comment #15)
> Both GTK and WPE ports already have an ImageDecoder superclass in
> Source/WebCore/platform/image-decoders. I guess that adding a new
> ImageDecoder definition in Source/WebCore/platform is what is causing WPE
> and GTK bots to fail.

Yep. I may have to integrate the two before landing.

[Said Abou-Hallawa]

(In reply to Jer Noble from comment #16)
> (In reply to Miguel Gomez from comment #15)
> > Both GTK and WPE ports already have an ImageDecoder superclass in
> > Source/WebCore/platform/image-decoders. I guess that adding a new
> > ImageDecoder definition in Source/WebCore/platform is what is causing WPE
> > and GTK bots to fail.
> 
> Yep. I may have to integrate the two before landing.

Another way to write this patch can be the following structure:

Source/WebCore/platform/graphics/BaseImageDecoder.h

class BaseImageDecoder : public ThreadSafeRefCounted<BaseImageDecoder> {
    // Add the virtual functions here.
};


Source/WebCore/platform/graphics/cg/ImageDecoderCG.h

class ImageDecoder : public BaseImageDecoder {
    // Override the virtual functions here.
};

[Jer Noble]

(In reply to Said Abou-Hallawa from comment #17)
> (In reply to Jer Noble from comment #16)
> > (In reply to Miguel Gomez from comment #15)
> > > Both GTK and WPE ports already have an ImageDecoder superclass in
> > > Source/WebCore/platform/image-decoders. I guess that adding a new
> > > ImageDecoder definition in Source/WebCore/platform is what is causing WPE
> > > and GTK bots to fail.
> > 
> > Yep. I may have to integrate the two before landing.
> 
> Another way to write this patch can be the following structure:
> 
> Source/WebCore/platform/graphics/BaseImageDecoder.h
> 
> class BaseImageDecoder : public ThreadSafeRefCounted<BaseImageDecoder> {
>     // Add the virtual functions here.
> };
> 
> 
> Source/WebCore/platform/graphics/cg/ImageDecoderCG.h
> 
> class ImageDecoder : public BaseImageDecoder {
>     // Override the virtual functions here.
> };

True, but that would probably require changing all the references in ImageSource, ImageFrameCache, etc. from ImageDecoder to BaseImageDecoder.

[Jer Noble]

Created attachment 320674 [details]
Patch for landing

[Jer Noble]

Created attachment 320676 [details]
Patch for landing

[Jer Noble]

Created attachment 320682 [details]
Patch for landing

[Jer Noble]

Created attachment 320686 [details]
Patch for landing

[Build Bot]

Attachment 320686 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:35:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:116:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 5 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 320693 [details]
Patch for landing

[Build Bot]

Attachment 320693 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:35:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 320694 [details]
Patch for landing

[Build Bot]

Attachment 320694 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:54:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:70:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:71:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:39:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:50:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:52:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:60:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:62:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:75:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:76:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:81:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:87:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:91:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:94:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:94:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:133:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:137:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:142:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:148:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:164:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:168:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:169:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:172:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:173:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:174:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:184:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:209:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:209:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:210:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:211:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:212:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:237:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:247:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:256:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:260:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:261:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:262:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:263:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:291:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:300:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:305:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:320:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:327:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:343:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:350:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:55:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:100:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:101:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:106:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:107:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:113:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:117:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:118:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:129:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:49:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:63:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:64:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:68:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:69:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:72:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:73:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 72 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

(In reply to Build Bot from comment #27)
> Attachment 320694 [details] did not pass style-queue:
> 

(snip). Ugh.

[Jer Noble]

Created attachment 320695 [details]
Patch for landing

[Build Bot]

Attachment 320695 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:87:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:91:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 12 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 320699 [details]
Patch for landing

[Build Bot]

Attachment 320699 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:87:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:91:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 12 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 320707 [details]
Patch for landing

[Build Bot]

Attachment 320707 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:87:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:91:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 12 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Okay, the GTK+ and WPE bots are happy; now to work on the Win bot.

[Jer Noble]

(In reply to Jer Noble from comment #35)
> Okay, the GTK+ and WPE bots are happy; now to work on the Win bot.

Actually, the Win bot might be happy too, judging by the immediately prior patch for landing.

[Jer Noble]

Okay, looks like the GTK+, WPE, and Win bots are all happy. Said, Miguel, would you mind looking this over and giving a LGTM (or not)? I'm specifically interested in what you think of:

- the rename of the image-decoders base class from ImageDecoder -> ScalableImageDecoder
- the changes to ScalableImageDecoder::frameIsCompleteAtIndex(...), ScalableImageDecoder::frameDurationAtIndex(...) to make them const-correct.

[Said Abou-Hallawa]

Comment on attachment 320707 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/ImageDecoder.cpp:47
> +#if USE(CG)
> +    return ImageDecoderCG::create(data, alphaOption, gammaAndColorProfileOption);
> +#elif USE(DIRECT2D)
> +    return ImageDecoderDirect2D::create(data, alphaOption, gammaAndColorProfileOption);
> +#else
> +    return ScalableImageDecoder::create(data, alphaOption, gammaAndColorProfileOption);
> +#endif

I do not like using different names for different platforms. I would suggest using the same name for all platforms, e.g. PlatformImageDecoder. In this case ImageDecoderCG.h and ImageDecoderCG.cpp will have the declaration and the implementation of PlatformImageDecoder for cg. ImageSource and ImageFrameCache should change ImageDecoder to PlatformImageDecoder. And no need for this file. ImageSource::ensureDecoderAvailable() will call PlatformImageDecoder::create() which will be implemented by all platforms.

> Source/WebCore/platform/graphics/ImageDecoder.h:54
> +    virtual String uti() const = 0;

No need for this function in the base class. It is only implemented by the CG ImageDecoder and it is called form CG code only.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.h:32
> +class ImageDecoderCG : public ImageDecoder {

Should we add final here? class ImageDecoderCG final : public ImageDecoder

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:33
> +#include "ImageSource.h"

ImageDecoder should not know anything about ImageSource. I do not think there is a need for this header file here.

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:40
> +class ImageDecoderDirect2D : public ImageDecoder {

final?

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:52
>      String filenameExtension() const;

I was surprised that the Windows port built fine while this function does not have the keyword "override" or "final". Then I realized that this file does not compile on any port. So please add final to all the virtual functions in this header file.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:173
> +bool ScalableImageDecoder::frameIsCompleteAtIndex(size_t index) const

I do not think this function can be const easily. This function has to call frameBufferAtIndex() which is not const and it will be very difficult to make this one const. You may end up marking all the functions in all the classes to be const and all the members to be mutable.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177
> -    ImageFrame* buffer = frameBufferAtIndex(index);
> -    return buffer && buffer->isComplete();
> +    if (m_frameBufferCache.size() <= index)
> +        return true;
> +    return m_frameBufferCache[index].isComplete();

I do not understand this change. Why do we return true if index >= m_frameBufferCache.size()? Animated images may change the frameCount as more is received. So if you call frameIsCompleteAtIndex() for frame say 100 while m_frameBufferCache.size() is 90, we should return false. The reason is as more data is received, the frameCount can change to say 120 frames. Once the frameCount changes, m_frameBufferCache will be grown to 120. And in this case the frame will be incomplete till its data is received.

Also why did you replace calling frameBufferAtIndex() to directly accessing m_frameBufferCache? frameBufferAtIndex() does lazy decoding and caching for the requested frame. So if you call frameBufferAtIndex(), more frames will be decoded and cached. If you access m_frameBufferCache, you will not get up-to-date decoded frames. Look at BMPImageDecoder::frameBufferAtIndex() and you will see that it calls decode() if the frame is not complete.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:-199
> -    ImageFrame* buffer = frameBufferAtIndex(index);
> -    if (!buffer || buffer->isInvalid())

Ditto.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206
> -    const float duration = buffer->duration() / 1000.0f;
> +    const float duration = m_frameBufferCache[index].duration();

Why do we return milli-seconds instead of seconds?

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:65
> +    String filenameExtension() const override = 0;

I do not think you need to override a pure virtual function by another pure virtual function.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:66
> +    String uti() const override { return emptyString(); }

No need for this function if you remove it from the base class.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:169
> +    size_t bytesDecodedToDetermineProperties() const override { return 0; }

Should be final.

> Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:52
> +    String filenameExtension() const override { return ASCIILiteral("bmp"); }
> +    void setData(SharedBuffer&, bool allDataReceived) override;
> +    ImageFrame* frameBufferAtIndex(size_t index) override;
> +    // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid
> +    // accessing deleted memory, especially when calling this from inside
> +    // BMPImageReader!
> +    bool setFailed() override;

I think our coding style states that as long as the class is marked as final, all the virtual functions should be marked final as well.

> Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:31
>  #ifndef BMPImageReader_h
>  #define BMPImageReader_h

Can we use pragma once?

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:58
> +    String filenameExtension() const override { return ASCIILiteral("gif"); }
> +    void setData(SharedBuffer& data, bool allDataReceived) override;
> +    bool setSize(const IntSize&) override;
> +    size_t frameCount() const override;
> +    RepetitionCount repetitionCount() const override;
> +    ImageFrame* frameBufferAtIndex(size_t index) override;
> +    // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid
> +    // accessing deleted memory, especially when calling this from inside
> +    // GIFImageReader!
> +    bool setFailed() override;
> +    void clearFrameBufferCache(size_t clearBeforeFrame) override;

Ditto. override -> final.

> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:59
> +    std::optional<IntPoint> hotSpot() const override;

Ditto. override -> final.

[Said Abou-Hallawa]

Comment on attachment 320707 [details]
Patch for landing

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

>> Source/WebCore/platform/graphics/ImageDecoder.cpp:47
>> +#endif
> 
> I do not like using different names for different platforms. I would suggest using the same name for all platforms, e.g. PlatformImageDecoder. In this case ImageDecoderCG.h and ImageDecoderCG.cpp will have the declaration and the implementation of PlatformImageDecoder for cg. ImageSource and ImageFrameCache should change ImageDecoder to PlatformImageDecoder. And no need for this file. ImageSource::ensureDecoderAvailable() will call PlatformImageDecoder::create() which will be implemented by all platforms.

I think I am wrong regarding the changes in ImageSource and ImageFrameCache. I think we should keep ImageSource and ImageFrameCache reference ImageDecoder only if we want to allow more subclassing.

[Jer Noble]

(In reply to Said Abou-Hallawa from comment #38)
> Comment on attachment 320707 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320707&action=review
> 
> > Source/WebCore/platform/graphics/ImageDecoder.cpp:47
> > +#if USE(CG)
> > +    return ImageDecoderCG::create(data, alphaOption, gammaAndColorProfileOption);
> > +#elif USE(DIRECT2D)
> > +    return ImageDecoderDirect2D::create(data, alphaOption, gammaAndColorProfileOption);
> > +#else
> > +    return ScalableImageDecoder::create(data, alphaOption, gammaAndColorProfileOption);
> > +#endif
> 
> I do not like using different names for different platforms. I would suggest
> using the same name for all platforms, e.g. PlatformImageDecoder. In this
> case ImageDecoderCG.h and ImageDecoderCG.cpp will have the declaration and
> the implementation of PlatformImageDecoder for cg. ImageSource and
> ImageFrameCache should change ImageDecoder to PlatformImageDecoder. And no
> need for this file. ImageSource::ensureDecoderAvailable() will call
> PlatformImageDecoder::create() which will be implemented by all platforms.

That only makes sense if every platform will have a single ImageDecoder, which isn't the case for GTK+, and soon won't be the case for Cocoa. Also, I don't like that the name of the class contained within the file will be different than the name of the file itself. It would also mean that no port could ever use both the CG and the BMP decoders simultaneously (for example).

> > Source/WebCore/platform/graphics/ImageDecoder.h:54
> > +    virtual String uti() const = 0;
> 
> No need for this function in the base class. It is only implemented by the
> CG ImageDecoder and it is called form CG code only.

Then it can have a default return value, rather than be a pure virtual.

> > Source/WebCore/platform/graphics/cg/ImageDecoderCG.h:32
> > +class ImageDecoderCG : public ImageDecoder {
> 
> Should we add final here? class ImageDecoderCG final : public ImageDecoder

Sure.

> > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:33
> > +#include "ImageSource.h"
> 
> ImageDecoder should not know anything about ImageSource. I do not think
> there is a need for this header file here.

Ok.

> > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:40
> > +class ImageDecoderDirect2D : public ImageDecoder {
> 
> final?

Sure.

> > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:52
> >      String filenameExtension() const;
> 
> I was surprised that the Windows port built fine while this function does
> not have the keyword "override" or "final". Then I realized that this file
> does not compile on any port. So please add final to all the virtual
> functions in this header file.

Ok.

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:173
> > +bool ScalableImageDecoder::frameIsCompleteAtIndex(size_t index) const
> 
> I do not think this function can be const easily.

Conceptually, this function should be const. In no way should asking whether a frame is complete mutate the underlying class.  If this function is expected to mutate, it should be named "ensureFrameIsCompleteAtIndex(...)".  

> This function has to call
> frameBufferAtIndex() which is not const and it will be very difficult to
> make this one const.

In this patch, the ScalableImageDecoder no longer calls frameBufferAtIndex(), it asks the m_frameBufferCache whether it the frame at that index is complete.  That may be incorrect.  But I think we need a better explanation about why asking whether a frame is complete will force the decoder to attempt to decode a frame.

> You may end up marking all the functions in all the
> classes to be const and all the members to be mutable.
> 
> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177
> > -    ImageFrame* buffer = frameBufferAtIndex(index);
> > -    return buffer && buffer->isComplete();
> > +    if (m_frameBufferCache.size() <= index)
> > +        return true;
> > +    return m_frameBufferCache[index].isComplete();
> 
> I do not understand this change. Why do we return true if index >=
> m_frameBufferCache.size()?

Whoops, that should be "return false".

> Animated images may change the frameCount as more
> is received. So if you call frameIsCompleteAtIndex() for frame say 100 while
> m_frameBufferCache.size() is 90, we should return false. The reason is as
> more data is received, the frameCount can change to say 120 frames. Once the
> frameCount changes, m_frameBufferCache will be grown to 120. And in this
> case the frame will be incomplete till its data is received.
> 
> Also why did you replace calling frameBufferAtIndex() to directly accessing
> m_frameBufferCache? frameBufferAtIndex() does lazy decoding and caching for
> the requested frame. So if you call frameBufferAtIndex(), more frames will
> be decoded and cached. If you access m_frameBufferCache, you will not get
> up-to-date decoded frames. Look at BMPImageDecoder::frameBufferAtIndex() and
> you will see that it calls decode() if the frame is not complete.

That is an idiosyncrasy of the decoders in image-decoders/ and is not the behavior of either of the other decoders in the tree. If asking whether a frame is complete is supposed to kick off a background decode, then the function is named incorrectly.

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:-199
> > -    ImageFrame* buffer = frameBufferAtIndex(index);
> > -    if (!buffer || buffer->isInvalid())
> 
> Ditto.

The answer is the same here. Asking for a frames' duration should not kick off a background decode of the frame.  If it should, that should be the API, and not a side-effect of asking.

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206
> > -    const float duration = buffer->duration() / 1000.0f;
> > +    const float duration = m_frameBufferCache[index].duration();
> 
> Why do we return milli-seconds instead of seconds?

Why is the ImageFrame have different time units than ImageDecoder?

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:65
> > +    String filenameExtension() const override = 0;
> 
> I do not think you need to override a pure virtual function by another pure
> virtual function.

True, this is an oversight. Removed.

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:66
> > +    String uti() const override { return emptyString(); }
> 
> No need for this function if you remove it from the base class.

Or move the default there.

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:169
> > +    size_t bytesDecodedToDetermineProperties() const override { return 0; }
> 
> Should be final.
> 
> > Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:52
> > +    String filenameExtension() const override { return ASCIILiteral("bmp"); }
> > +    void setData(SharedBuffer&, bool allDataReceived) override;
> > +    ImageFrame* frameBufferAtIndex(size_t index) override;
> > +    // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid
> > +    // accessing deleted memory, especially when calling this from inside
> > +    // BMPImageReader!
> > +    bool setFailed() override;
> 
> I think our coding style states that as long as the class is marked as
> final, all the virtual functions should be marked final as well.

Ok.

> > Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:31
> >  #ifndef BMPImageReader_h
> >  #define BMPImageReader_h
> 
> Can we use pragma once?

Yes.  All these files violate our current coding guidelines in one way or another.

> > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:58
> > +    String filenameExtension() const override { return ASCIILiteral("gif"); }
> > +    void setData(SharedBuffer& data, bool allDataReceived) override;
> > +    bool setSize(const IntSize&) override;
> > +    size_t frameCount() const override;
> > +    RepetitionCount repetitionCount() const override;
> > +    ImageFrame* frameBufferAtIndex(size_t index) override;
> > +    // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid
> > +    // accessing deleted memory, especially when calling this from inside
> > +    // GIFImageReader!
> > +    bool setFailed() override;
> > +    void clearFrameBufferCache(size_t clearBeforeFrame) override;
> 
> Ditto. override -> final.

Ok.

> > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:59
> > +    std::optional<IntPoint> hotSpot() const override;
> 
> Ditto. override -> final.

Ok.

[Jer Noble]

(In reply to Jer Noble from comment #40)
> (In reply to Said Abou-Hallawa from comment #38)
> > Comment on attachment 320707 [details]
> > > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:33
> > > +#include "ImageSource.h"
> > 
> > ImageDecoder should not know anything about ImageSource. I do not think
> > there is a need for this header file here.
> 
> Ok.

Wait, if no port uses this file, why is it still in the tree?

[Jer Noble]

(In reply to Jer Noble from comment #41)
> Wait, if no port uses this file, why is it still in the tree?

Actually, this was in reference to the comment:

> > I was surprised that the Windows port built fine while this function does
> > not have the keyword "override" or "final". Then I realized that this file
> > does not compile on any port. So please add final to all the virtual
> > functions in this header file.

If no port uses this file, we should just remove it, rather than incur the ongoing maintenance burden of changing it whenever the interface in ImageDecoder changes.

[Jer Noble]

Created attachment 320869 [details]
Patch for landing

[Build Bot]

Attachment 320869 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/ImageDecoder.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 13 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

(In reply to Jer Noble from comment #40)
> (In reply to Said Abou-Hallawa from comment #38)
> > Comment on attachment 320707 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320707&action=review
> > 
> > 
> > Also why did you replace calling frameBufferAtIndex() to directly accessing
> > m_frameBufferCache? frameBufferAtIndex() does lazy decoding and caching for
> > the requested frame. So if you call frameBufferAtIndex(), more frames will
> > be decoded and cached. If you access m_frameBufferCache, you will not get
> > up-to-date decoded frames. Look at BMPImageDecoder::frameBufferAtIndex() and
> > you will see that it calls decode() if the frame is not complete.
> 
> That is an idiosyncrasy of the decoders in image-decoders/ and is not the
> behavior of either of the other decoders in the tree. If asking whether a
> frame is complete is supposed to kick off a background decode, then the
> function is named incorrectly.

Looking into this more, I'm pretty sure that your interpretation is incorrect. ImageFrameCache will do it's own synchronous or asynchronous decoding of frames as needed, either in ImageFrameCache::frameAtIndexCacheIfNeeded(...) or ImageFrameCache::requestFrameAsyncDecodingAtIndex(...).  If calling ScalableImageDecoder::frameIsCompleteAtIndex(...) will do a lazy, synchronous decode of an image, that would entirely violate the intent of the ImageFrameCache::requestFrameAsyncDecodingAtIndex(...) method, which calls frameIsCompleteAtIndex(...) before enqueueing a decode.

Now, this may have been broken this entire time. But as far as I can see, the old behavior was incorrect, and the new behavior is correct.

[Said Abou-Hallawa]

Comment on attachment 320869 [details]
Patch for landing

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

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177
> -    ImageFrame* buffer = frameBufferAtIndex(index);
> -    return buffer && buffer->isComplete();
> +    if (m_frameBufferCache.size() <= index)
> +        return false;
> +    return m_frameBufferCache[index].isComplete();

Maybe the function name is inappropriate but the expectation is this class does a lazy decoding. This means when it receives new encoded data, it basically stores it but it does nothing with it. Only when it needs to answer a question whose answer may be in the already received data, it started the decoding process.  

So I think this change is incorrect. I would expect this change will break all the image layout tests on GTK.

> Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206
> -    const float duration = buffer->duration() / 1000.0f;
> +    const float duration = m_frameBufferCache[index].duration();

Please notice that ImageFrame is used in two places for two different reasons. 

-- ScalableImageDecoder builds its own cache of ImageFrames to help answering the frame query metadata questions and to decode the image frames quickly.
-- ImageFrameCache builds its own cache of ImageFrames to guarantee the availability of decode frames for the visible image.

Notice also:

-- We used to have different but similar structures one was named FrameData and which used to live in BitmapImage.h and the other one was called ImageFrame and was living in Source/WebCore/platform/image-decoders/ImageDecoder.h. I managed to merge the two structures after many re-factoring patches. My plan was to eliminate this double caching and have the GTK decoders have access to the ImageFrameCache which is also created by the ImageSource.

We have one conceptual difference between the two uses of ImageFrame:
-- If the ImageFrame is in ImageFrameCache, the duration is in seconds. 
-- If the ImageFrame is in ScalableImageDecoder, the duration is in milli seconds.

I know it is confusing and I caused much of it, but if I have enough time I will fix this issue.

So I think this change is incorrect. If you want to unify the units of the duration in the ImageFrame we can do the following:

-- Change the type of ImageFrame::m_duration to be Seconds.
-- Ensure all the GTK decoders sets the milliseconds of m_duration, while the GC decoder sets the seconds of m_duration.

[Jer Noble]

(In reply to Said Abou-Hallawa from comment #46)
> Comment on attachment 320869 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320869&action=review
> 
> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177
> > -    ImageFrame* buffer = frameBufferAtIndex(index);
> > -    return buffer && buffer->isComplete();
> > +    if (m_frameBufferCache.size() <= index)
> > +        return false;
> > +    return m_frameBufferCache[index].isComplete();
> 
> Maybe the function name is inappropriate but the expectation is this class
> does a lazy decoding. This means when it receives new encoded data, it
> basically stores it but it does nothing with it. Only when it needs to
> answer a question whose answer may be in the already received data, it
> started the decoding process.  
> 
> So I think this change is incorrect. I would expect this change will break
> all the image layout tests on GTK.

As I pointed out above, this can't be true, as it would mean that async decoding was broken.  The only call sites for frameIsCompleteAtIndex(...) are in ImageFrameCache, and it definitely doesn't expect decoding to occur, and it will explicitly start decoding, explicitly, itself.

> > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206
> > -    const float duration = buffer->duration() / 1000.0f;
> > +    const float duration = m_frameBufferCache[index].duration();
> 
> Please notice that ImageFrame is used in two places for two different
> reasons. 
> 
> -- ScalableImageDecoder builds its own cache of ImageFrames to help
> answering the frame query metadata questions and to decode the image frames
> quickly.
> -- ImageFrameCache builds its own cache of ImageFrames to guarantee the
> availability of decode frames for the visible image.

Ok.

> Notice also:
> 
> -- We used to have different but similar structures one was named FrameData
> and which used to live in BitmapImage.h and the other one was called
> ImageFrame and was living in
> Source/WebCore/platform/image-decoders/ImageDecoder.h. I managed to merge
> the two structures after many re-factoring patches. My plan was to eliminate
> this double caching and have the GTK decoders have access to the
> ImageFrameCache which is also created by the ImageSource.
> 
> We have one conceptual difference between the two uses of ImageFrame:
> -- If the ImageFrame is in ImageFrameCache, the duration is in seconds. 
> -- If the ImageFrame is in ScalableImageDecoder, the duration is in milli
> seconds.
> 
> I know it is confusing and I caused much of it, but if I have enough time I
> will fix this issue.
> 
> So I think this change is incorrect. If you want to unify the units of the
> duration in the ImageFrame we can do the following:

Okay, for now, we can just change the return type of ImageDecoder::frameDurationAtIndex(...) to be Seconds, and each decoder can handle converting to seconds correctly.  In a future patch, ImageFrame can be cleaned up. I don't want to get into rewriting the duration logic of each GTK+ decoder.

> -- Change the type of ImageFrame::m_duration to be Seconds.
> -- Ensure all the GTK decoders sets the milliseconds of m_duration, while
> the GC decoder sets the seconds of m_duration.

[Jer Noble]

(In reply to Jer Noble from comment #47)
> (In reply to Said Abou-Hallawa from comment #46)
> > Comment on attachment 320869 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320869&action=review
> > 
> > So I think this change is incorrect. I would expect this change will break
> > all the image layout tests on GTK.
> 
> As I pointed out above, this can't be true, as it would mean that async
> decoding was broken.  The only call sites for frameIsCompleteAtIndex(...)
> are in ImageFrameCache, and it definitely doesn't expect decoding to occur,
> and it will explicitly start decoding, explicitly, itself.

Anyway, this should be easy to test; I'll install a GTK+ VM and see.

[Jer Noble]

(In reply to Jer Noble from comment #48)
> (In reply to Jer Noble from comment #47)
> > (In reply to Said Abou-Hallawa from comment #46)
> > > Comment on attachment 320869 [details]
> > > Patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=320869&action=review
> > > 
> > > So I think this change is incorrect. I would expect this change will break
> > > all the image layout tests on GTK.
> > 
> > As I pointed out above, this can't be true, as it would mean that async
> > decoding was broken.  The only call sites for frameIsCompleteAtIndex(...)
> > are in ImageFrameCache, and it definitely doesn't expect decoding to occur,
> > and it will explicitly start decoding, explicitly, itself.
> 
> Anyway, this should be easy to test; I'll install a GTK+ VM and see.

So I finally got a GTK+ build working in a VM, and Said is indeed correct that this change causes animating images to stop working with non-CG decoders.  After digging around, I think I understand why.

The underlying problem is the disconnect in what the BitmapImage means by "frameIsCompleteAtIndex()", and what the non-CG decoders mean by "frameIsCompleteAtIndex()". The former wants to know if the decoder has been fed enough data to allow decoding of the frame to succeed.  The latter wants to answer whether the frame has been decoded yet.  ImageDecoderCG::frameIsCompleteAtIndex() agrees with BitmapImage.

So in BitmapImage::internalStartAnimation(), the image doesn't want to start a decode operation at an index for which the decoder doesn't have enough data, so it bails early if frameIsCompleteAtIndex() returns false, and never enqueues a decode operation.  Meanwhile, the image-decoders/ decoders will all synchronously decode the image for index when asked frameIsCompleteAtIndex(), making any async decode operations entirely pointless.

So, what do we do?

As far as I can see, the image-decoders/ decoders need to be entirely rewritten to /parse/ the image data when that data is appended. Note /parse/ and not decode. They need to be able to answer the question of whether decoding can proceed without actually decoding first. Because right now, for non-CG ports, async decoding of GIFs (for example) is entirely broken and is most likely happening entirely on the main thread.

But, for the purposes of this patch, that's way to big a re-write for me to take on. I'll back out my change to ScalableImageDecoder::frameIsCompleteAtIndex() (without making it non-const; the bad design in the image-decoders/ decoders shouldn't force design decisions on the other decoders in the tree.)

[Jer Noble]

Created attachment 320995 [details]
Patch for landing

[Build Bot]

Attachment 320995 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/ImageDecoder.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 13 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to Jer Noble from comment #49)
> Because right now, for
> non-CG ports, async decoding of GIFs (for example) is entirely broken and is
> most likely happening entirely on the main thread.

GIFs have been totally broken for over a year now, see bug #176089. (I only reported it recently, but they broke sometime last summer or fall IIRC.)

Yes, it is pretty embarrassing that we have not fixed it in all that time.

[Jer Noble]

(In reply to Michael Catanzaro from comment #52)
> (In reply to Jer Noble from comment #49)
> > Because right now, for
> > non-CG ports, async decoding of GIFs (for example) is entirely broken and is
> > most likely happening entirely on the main thread.
> 
> GIFs have been totally broken for over a year now, see bug #176089. (I only
> reported it recently, but they broke sometime last summer or fall IIRC.)
> 
> Yes, it is pretty embarrassing that we have not fixed it in all that time.

Thanks for the extra context Michael. I'll update the FIXMEs in this patch to bug #176089.

[Jer Noble]

Created attachment 321093 [details]
Patch for landing

[Build Bot]

Attachment 321093 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/ImageDecoder.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 13 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 321093 [details]
Patch for landing

Clearing flags on attachment: 321093

Committed r222151: <http://trac.webkit.org/changeset/222151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694191>