200498 – Remove the dead code of ScalableImageDecoder for scaling

RESOLVED FIXED 200498

Remove the dead code of ScalableImageDecoder for scaling

https://bugs.webkit.org/show_bug.cgi?id=200498

Summary Remove the dead code of ScalableImageDecoder for scaling

Fujii Hironori

Reported 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

Attachments
Patch (23.79 KB, patch) 2019-08-20 03:30 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Patch (23.76 KB, patch) 2019-08-20 20:14 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Patch (23.85 KB, patch) 2019-08-21 19:28 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Fujii Hironori

Comment 1 2019-08-07 01:58:26 PDT

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

Fujii Hironori

Comment 2 2019-08-07 06:36:10 PDT

Annulen, are you using down scaling feature of ScalableImageDecoder?

Konstantin Tokarev

Comment 3 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.

Fujii Hironori

Comment 4 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.

Fujii Hironori

Comment 5 2019-08-20 03:30:55 PDT

Created attachment 376761 [details] Patch

Konstantin Tokarev

Comment 6 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

Loïc Yhuel

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

Fujii Hironori

Comment 8 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.

Fujii Hironori

Comment 9 2019-08-20 20:14:03 PDT

Created attachment 376840 [details] Patch * Addressed review feedbacks

Fujii Hironori

Comment 10 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.

Fujii Hironori

Comment 11 2019-08-21 14:29:17 PDT

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

Konstantin Tokarev

Comment 12 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

Fujii Hironori

Comment 13 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

Fujii Hironori

Comment 14 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.

Fujii Hironori

Comment 15 2019-08-21 19:28:37 PDT

Created attachment 376967 [details] Patch * Removed the nagative checks.

Daniel Bates

Comment 16 2019-08-21 20:08:16 PDT

Comment on attachment 376967 [details] Patch Looks sane to me. r=me

Fujii Hironori

Comment 17 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>

Fujii Hironori

Comment 18 2019-08-22 02:13:15 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 19 2019-08-22 02:14:18 PDT

<rdar://problem/54591083>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Images

Assignee

Fujii Hironori

Reported

2019-08-07 00:50 PDT

Modified

2019-08-22 02:14 PDT History

CC List

8 users Show

URL

Keywords InRadar

Depends on

Blocks

200962 200163

Dependencies

tree graph