Bug 200498

Summary: Remove the dead code of ScalableImageDecoder for scaling
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: ImagesAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, dbates, don.olmstead, loic.yhuel, ross.kirsling, sabouhallawa, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=179921
Bug Depends on:    
Bug Blocks: 200962, 200163    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Fujii Hironori 2019-08-07 00:50:26 PDT
Remove the dead code of ScalableImageDecoder for scaling

member variables m_maxNumPixels, m_scaled, m_scaledColumns and m_scaledRows are unused.

See Also: Bug 179921
Comment 1 Fujii Hironori 2019-08-07 01:58:26 PDT
This feature was added by
Bug 28308 – allow down-sampling images during decoding
Comment 2 Fujii Hironori 2019-08-07 06:36:10 PDT
Annulen, are you using down scaling feature of ScalableImageDecoder?
Comment 3 Konstantin Tokarev 2019-08-07 07:30:06 PDT
Qt port had support for IMAGE_DECODER_DOWN_SAMPLING, but after its removal in r225091 it seems that these variables are not used anywhere.
Comment 4 Fujii Hironori 2019-08-07 18:29:28 PDT
Got it.
JPEGImageDecoder::outputScanlines doesn't support the case out_color_space is BGRA and m_scaled is true.
It difficult to maintain dead code. Let's remove it.
If someone want to resurrect this feature, they should add a unit test.
Comment 5 Fujii Hironori 2019-08-20 03:30:55 PDT
Created attachment 376761 [details]
Patch
Comment 6 Konstantin Tokarev 2019-08-20 09:43:52 PDT
Comment on attachment 376761 [details]
Patch

r=me, but after these changes ScalableImageDecoder is not a meaningful name of the class anymore
Comment 7 Loïc Yhuel 2019-08-20 10:16:29 PDT
Comment on attachment 376761 [details]
Patch

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

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:565
> +        if (sourceY < 0)

This cannot happen, output_scanline is unsigned (JDIMENSION).

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:471
> +    if (rowIndex >= size().height())

I think this cannot happen now, the decoder will not give us a row outside the image height.
Comment 8 Fujii Hironori 2019-08-20 20:12:31 PDT
Comment on attachment 376761 [details]
Patch

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

Thank you for the review.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:565
>> +        if (sourceY < 0)
> 
> This cannot happen, output_scanline is unsigned (JDIMENSION).

Will fix.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:471
>> +    if (rowIndex >= size().height())
> 
> I think this cannot happen now, the decoder will not give us a row outside the image height.

See the above comment "libpng may send extra rows, ignore them to make our lives easier". It seems that I should keep this checking.
Comment 9 Fujii Hironori 2019-08-20 20:14:03 PDT
Created attachment 376840 [details]
Patch

* Addressed review feedbacks
Comment 10 Fujii Hironori 2019-08-20 20:23:29 PDT
(In reply to Konstantin Tokarev from comment #6)
> Comment on attachment 376761 [details]
> Patch
> 
> r=me, but after these changes ScalableImageDecoder is not a meaningful name
> of the class anymore

Agreed. Filed Bug 200962.
Comment 11 Fujii Hironori 2019-08-21 14:29:17 PDT
Konstantin, do you have a chance to review this patch again?
Comment 12 Konstantin Tokarev 2019-08-21 14:53:26 PDT
Comment on attachment 376840 [details]
Patch

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

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:216
>      if (rowBuffer.isEmpty() || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))

Condition (xBegin < 0) || (yBegin < 0) is always false, because xOffset and yOffset are unsigned. Also it should be possible to change types of all variables {x,y}{Begin,End} to unsigned and drop static_cast
Comment 13 Fujii Hironori 2019-08-21 19:22:40 PDT
Removing static_cast<int> reports the following compilation errors because std::min takes size_t and int.

> ..\Source\WebCore\platform\image-decoders\gif\GIFImageDecoder.cpp(214,21): error: no matching function for call to 'min'
>     unsigned xEnd = std::min(frameContext->xOffset + width, size().width());
>                     ^~~~~~~~
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4492,82): note: candidate template ignored: deduced conflicting types for parameter '_Ty' ('unsigned long long' vs. 'int')
> _Post_equal_to_(_Right < _Left ? _Right : _Left) _NODISCARD constexpr const _Ty&(min)(const _Ty& _Left,
>                                                                                  ^
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4484,26): note: candidate template ignored: could not match 'initializer_list<type-parameter-0-0>' against 'unsigned long long'
> _NODISCARD constexpr _Ty(min)(initializer_list<_Ty> _Ilist, _Pr _Pred) { // return leftmost/smallest
>                          ^
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4478,33): note: candidate function template not viable: requires 3 arguments, but 2 were provided
> _NODISCARD constexpr const _Ty&(min)(const _Ty& _Left, const _Ty& _Right, _Pr _Pred) _NOEXCEPT_COND(
>                                 ^
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4504,26): note: candidate function template not viable: requires single argument '_Ilist', but 2 arguments were provided
> _NODISCARD constexpr _Ty(min)(initializer_list<_Ty> _Ilist) { // return leftmost/smallest
>                          ^
> ..\..\Source\WebCore\platform\image-decoders\gif\GIFImageDecoder.cpp(215,21): error: no matching function for call to 'min'
>     unsigned yEnd = std::min(frameContext->yOffset + rowNumber + repeatCount, size().height());
>                     ^~~~~~~~
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4492,82): note: candidate template ignored: deduced conflicting types for parameter '_Ty' ('unsigned long long' vs. 'int')
> _Post_equal_to_(_Right < _Left ? _Right : _Left) _NODISCARD constexpr const _Ty&(min)(const _Ty& _Left,
>                                                                                  ^
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4484,26): note: candidate template ignored: could not match 'initializer_list<type-parameter-0-0>' against 'unsigned long long'
> _NODISCARD constexpr _Ty(min)(initializer_list<_Ty> _Ilist, _Pr _Pred) { // return leftmost/smallest
>                          ^
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4478,33): note: candidate function template not viable: requires 3 arguments, but 2 were provided
> _NODISCARD constexpr const _Ty&(min)(const _Ty& _Left, const _Ty& _Right, _Pr _Pred) _NOEXCEPT_COND(
>                                 ^
> C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.21.27702\include\algorithm(4504,26): note: candidate function template not viable: requires single argument '_Ilist', but 2 arguments were provided
> _NODISCARD constexpr _Ty(min)(initializer_list<_Ty> _Ilist) { // return leftmost/smallest
Comment 14 Fujii Hironori 2019-08-21 19:26:44 PDT
Comment on attachment 376840 [details]
Patch

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

>> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:216
>>      if (rowBuffer.isEmpty() || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))
> 
> Condition (xBegin < 0) || (yBegin < 0) is always false, because xOffset and yOffset are unsigned. Also it should be possible to change types of all variables {x,y}{Begin,End} to unsigned and drop static_cast

Good catch, I will remove the (xBegin < 0) || (yBegin < 0).
I don't think using int for all variables {x,y}{Begin,End} is a bad idea in this case because the image size are represented as IntRect and IntSize.
Comment 15 Fujii Hironori 2019-08-21 19:28:37 PDT
Created attachment 376967 [details]
Patch

* Removed the nagative checks.
Comment 16 Daniel Bates 2019-08-21 20:08:16 PDT
Comment on attachment 376967 [details]
Patch

Looks sane to me. r=me
Comment 17 Fujii Hironori 2019-08-22 02:13:11 PDT
Comment on attachment 376967 [details]
Patch

Clearing flags on attachment: 376967

Committed r248998: <https://trac.webkit.org/changeset/248998>
Comment 18 Fujii Hironori 2019-08-22 02:13:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2019-08-22 02:14:18 PDT
<rdar://problem/54591083>