Bug 200498 – Remove the dead code of ScalableImageDecoder for scaling

• Component: Images
• Status: RESOLVED FIXED

[Fujii Hironori]

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

[Fujii Hironori]

This feature was added by
Bug 28308 – allow down-sampling images during decoding

[Fujii Hironori]

Annulen, are you using down scaling feature of ScalableImageDecoder?

[Konstantin Tokarev]

Qt port had support for IMAGE_DECODER_DOWN_SAMPLING, but after its removal in r225091 it seems that these variables are not used anywhere.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 376761 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 376761 [details]
Patch

r=me, but after these changes ScalableImageDecoder is not a meaningful name of the class anymore

[Loïc Yhuel]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 376840 [details]
Patch

* Addressed review feedbacks

[Fujii Hironori]

(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.

[Fujii Hironori]

Konstantin, do you have a chance to review this patch again?

[Konstantin Tokarev]

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

[Fujii Hironori]

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

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 376967 [details]
Patch

* Removed the nagative checks.

[Daniel Bates]

Comment on attachment 376967 [details]
Patch

Looks sane to me. r=me

[Fujii Hironori]

Comment on attachment 376967 [details]
Patch

Clearing flags on attachment: 376967

Committed r248998: <https://trac.webkit.org/changeset/248998>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54591083>