Summary: | [GPU Process] Make ImageBuffer calculate its memoryCost() from its size | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||
Component: | Canvas | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | dino, mmaxfield, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222880 | ||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2021-04-07 14:53:26 PDT
Created attachment 425450 [details]
Patch
Created attachment 425460 [details]
Patch
Created attachment 425465 [details]
Patch
Created attachment 425473 [details]
Patch
Created attachment 425476 [details]
Patch
Comment on attachment 425476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425476&action=review > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:40 > - IntSize backendSize = IntSize(scaledSize); > - > - Checked<unsigned, RecordOverflow> bytesPerRow = 4 * Checked<unsigned, RecordOverflow>(backendSize.width()); > - if (bytesPerRow.hasOverflowed()) > - return { }; > - > - CheckedSize numBytes = Checked<unsigned, RecordOverflow>(backendSize.height()) * bytesPerRow; > - if (numBytes.hasOverflowed()) > - return { }; > + return IntSize(scaledSize); Don't we need CheckedArithmetic to ensure that scaledSize doesn't overflow an IntSize? parameters.resolutionScale could be large. > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:69 > + Checked<unsigned, RecordOverflow> bytesPerRow = (Checked<unsigned, RecordOverflow>(backendSize.width()) * 4); > + ASSERT(!bytesPerRow.hasOverflowed()); The point of Checked<> is to catch things in release builds. > Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:48 > + Checked<unsigned, RecordOverflow> bytesPerRow = 4 * Checked<unsigned, RecordOverflow>(backendSize.width()); Maybe now, certainly in the future, assuming a pixel is 4 bytes is wrong. Comment on attachment 425476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425476&action=review >> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:40 >> + return IntSize(scaledSize); > > Don't we need CheckedArithmetic to ensure that scaledSize doesn't overflow an IntSize? parameters.resolutionScale could be large. I think we check this above in if (scaledSize.isEmpty() || !scaledSize.isExpressibleAsIntSize()) return { }; >> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:69 >> + ASSERT(!bytesPerRow.hasOverflowed()); > > The point of Checked<> is to catch things in release builds. Yes you are right. unsafeGet() below will call RecordOverflow::crash() if bytesPerRow.hasOverflowed() is true. >> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:48 >> + Checked<unsigned, RecordOverflow> bytesPerRow = 4 * Checked<unsigned, RecordOverflow>(backendSize.width()); > > Maybe now, certainly in the future, assuming a pixel is 4 bytes is wrong. There are many places in the ImageBuffer code assume a pixel is 4 bytes. I think making bytesPerPixel be color-space dependent needs a lot of changes. Created attachment 425659 [details]
Patch
Created attachment 425715 [details]
Patch
Committed r275905 (236469@main): <https://commits.webkit.org/236469@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425715 [details]. |