Bug 163696 - Extract common code for computing byte count for images
Summary: Extract common code for computing byte count for images
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
Depends on: 163670
  Show dependency treegraph
Reported: 2016-10-19 14:53 PDT by David Kilzer (:ddkilzer)
Modified: 2016-10-20 15:35 PDT (History)
10 users (show)

See Also:

Patch v1 (19.32 KB, patch)
2016-10-19 15:01 PDT, David Kilzer (:ddkilzer)
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-10-19 14:53:50 PDT
There are many places where we multiply IntSize.width() * IntSize.height() * 4, so we should pull this into a utility method.

I chose Color.h mostly because it's where "typedef unsigned RGBA32" lives.
Comment 1 David Kilzer (:ddkilzer) 2016-10-19 15:01:02 PDT
Created attachment 292116 [details]
Patch v1
Comment 2 Simon Fraser (smfr) 2016-10-19 15:22:21 PDT
Comment on attachment 292116 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=292116&action=review

> Source/WebCore/platform/graphics/Color.h:402
> +inline unsigned numBytesWithColor(const IntSize& size)
> +{
> +    Checked<unsigned> result = Checked<unsigned>(size.area()) * sizeof(RGBA32);
> +    return result.unsafeGet();
> +}

I don't think this is a good place, and encourages people to assume sizeof(pixel) == 4 but that might change.

I think each image or buffer object should have a bytesSize() accessor.
Comment 3 David Kilzer (:ddkilzer) 2016-10-20 13:42:44 PDT
Broke out PDFDocumentImage.cpp changes into:

Bug 163757: Use IntSize::unclampedArea() in PDFDocumentImage::updateCachedImageIfNeeded()
Comment 4 David Kilzer (:ddkilzer) 2016-10-20 15:34:05 PDT
Broke out use of checked arithmetic in IntSize::area() as:

Bug 163762: IntSize::area() should used checked arithmetic
Comment 5 David Kilzer (:ddkilzer) 2016-10-20 15:35:29 PDT
Moving to RESOLVED/WONTFIX since there probably needs to be more discussion here before a change is made, and I'm not an area expert.