Bug 200498

Summary:

Remove the dead code of ScalableImageDecoder for scaling

Product:

WebKit

Reporter:

Fujii Hironori <Hironori.Fujii>

Component:

Images

Assignee:

Fujii Hironori <Hironori.Fujii>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, dbates, don.olmstead, loic.yhuel, ross.kirsling, sabouhallawa, webkit-bug-importer, zan

Priority:

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

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.