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: | 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
Fujii Hironori
2019-08-07 00:50:26 PDT
This feature was added by Bug 28308 – allow down-sampling images during decoding Annulen, are you using down scaling feature of ScalableImageDecoder? Qt port had support for IMAGE_DECODER_DOWN_SAMPLING, but after its removal in r225091 it seems that these variables are not used anywhere. 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. Created attachment 376761 [details]
Patch
Comment on attachment 376761 [details]
Patch
r=me, but after these changes ScalableImageDecoder is not a meaningful name of the class anymore
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 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. Created attachment 376840 [details]
Patch
* Addressed review feedbacks
(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. Konstantin, do you have a chance to review this patch again? 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 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 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. Created attachment 376967 [details]
Patch
* Removed the nagative checks.
Comment on attachment 376967 [details]
Patch
Looks sane to me. r=me
Comment on attachment 376967 [details] Patch Clearing flags on attachment: 376967 Committed r248998: <https://trac.webkit.org/changeset/248998> All reviewed patches have been landed. Closing bug. |