Bug 79395 – Add ImageBuffer support for having a hi-res backing store

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Matthew Delaney]

Many of the clients of ImageBuffer create oversized versions and then scale up their contexts to have a high-res version of the ImageBuffer. This logic is being duplicated in many locations and should just be moved down into ImageBuffer. This bug tracks building hi-res support directly into ImageBuffer so that the ImageBuffer internals can deal with the scale factor directly.

One example of a client whose code would become much cleaner by adopting this would be <canvas>. I'll file a follow up bug to track <canvas> adopting this change in factor of mucking with the internals of the ImageBuffer directly, as it currently does.

[Matthew Delaney]

Created attachment 128550 [details]
Patch

[Matthew Delaney]

(In reply to comment #1)
> Created an attachment (id=128550) [details]
> Patch

Note: I still need to update the other platforms, though the bots are pretty broken right now anyhow.

[Philippe Normand]

Comment on attachment 128550 [details]
Patch

Attachment 128550 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11576426

[Matthew Delaney]

Comment on attachment 128550 [details]
Patch

That patch had some junk in it, reuploading in a min.

[Early Warning System Bot]

Comment on attachment 128550 [details]
Patch

Attachment 128550 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11610371

[WebKit Review Bot]

Comment on attachment 128550 [details]
Patch

Attachment 128550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11600444

[Matthew Delaney]

Created attachment 128570 [details]
Patch

[Matthew Delaney]

I reworked it to require less changes to other ports. So, this patch is EWS food.

[WebKit Review Bot]

Comment on attachment 128570 [details]
Patch

Attachment 128570 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11576463

[Matthew Delaney]

Created attachment 128576 [details]
Patch

[Matthew Delaney]

Created attachment 128586 [details]
Patch

[Matthew Delaney]

Had a mistype in the last patch.

Comment on attachment 128586 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        (WebCore::ImageBuffer::devicePixelRatio): Add a way to access if ratio.

“if”?

> Source/WebCore/ChangeLog:17
> +        (WebCore::ImageBuffer::dataSize): Differentiate between the logical size and the underlying data's size.

To me, “data size” strongly suggests “number of bytes”. How about “physicalSize” instead?

> Source/WebCore/ChangeLog:20
> +        and thus clearer use of logicalSize or deviceSize.

Here you call it “deviceSize” but I suspect you mean the same thing as “dataSize”.

> Source/WebCore/html/HTMLCanvasElement.cpp:527
> -    scriptExecutionContext()->globalData()->heap.reportExtraMemoryCost(m_imageBuffer->dataSize());
> +    size_t numBytes = 4 * m_imageBuffer->dataSize().width() * m_imageBuffer->dataSize().height();
> +    scriptExecutionContext()->globalData()->heap.reportExtraMemoryCost(numBytes);

Now I see that dataSize() currently does mean a size in bytes. I strongly suggest leaving it to mean that, and using a different name for the new function.

> Source/WebCore/platform/graphics/ImageBuffer.h:84
> +            dataSize.scale(devicePixelRatio);

Does this need to be rounded up?

> Source/WebCore/platform/graphics/ImageBuffer.h:91
> +            buf->context()->scale(FloatSize(devicePixelRatio, devicePixelRatio));
> +            buf->m_devicePixelRatio = devicePixelRatio;

Is there a reason why you didn’t just add the device pixel ratio to the ImageBuffer constructor?

> Source/WebCore/platform/graphics/ImageBuffer.h:105
> +        const IntSize logicalSize() const
> +        { 
> +            IntSize logicalSize = dataSize();
> +            logicalSize.scale(1 / devicePixelRatio());
> +            return logicalSize;
> +        }
> +

Because of rounding, this is not necessarily going to return the size you passed the ImageBuffer::create(). Isn’t this going to be inconvenient for clients?

> Source/WebCore/platform/graphics/ImageBuffer.h:160
> +        

Why the newline?

[Early Warning System Bot]

Comment on attachment 128586 [details]
Patch

Attachment 128586 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11603489

[Matthew Delaney]

Comment on attachment 128586 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        (WebCore::ImageBuffer::dataSize): Differentiate between the logical size and the underlying data's size.
> 
> To me, “data size” strongly suggests “number of bytes”. How about “physicalSize” instead?

How about "deviceSize" instead? Then it'll match the already-existing notion of "deviceScaleFactor".

>> Source/WebCore/ChangeLog:20
>> +        and thus clearer use of logicalSize or deviceSize.
> 
> Here you call it “deviceSize” but I suspect you mean the same thing as “dataSize”.

Yep, typo when I was already thinking of this as an alternate name! :-)

>> Source/WebCore/html/HTMLCanvasElement.cpp:527
>> +    scriptExecutionContext()->globalData()->heap.reportExtraMemoryCost(numBytes);
> 
> Now I see that dataSize() currently does mean a size in bytes. I strongly suggest leaving it to mean that, and using a different name for the new function.

That old declaration of dataSize() returned the number of bytes and this was the only call site. The new version of the method returns the size w.r.t pixels, not bytes.  So, I updated this to have a mult by 4 to calc the number of bytes for the purposes of this gc hook.

>> Source/WebCore/platform/graphics/ImageBuffer.h:84
>> +            dataSize.scale(devicePixelRatio);
> 
> Does this need to be rounded up?

Yea, good call. I'll fix this.

>> Source/WebCore/platform/graphics/ImageBuffer.h:91
>> +            buf->m_devicePixelRatio = devicePixelRatio;
> 
> Is there a reason why you didn’t just add the device pixel ratio to the ImageBuffer constructor?

Yea, I wanted to avoid having to modify all the platform impls ;-)

[Simon Fraser (smfr)]

Comment on attachment 128576 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.h:97
> +        bool devicePixelRatio() const { return m_devicePixelRatio; }

We use the term deviceScaleFactor() elsewhere. However, this is ImageBuffer's pixel ratio, not a device pixel ratio.

[Matthew Delaney]

Created attachment 128635 [details]
Patch

[Matthew Delaney]

> We use the term deviceScaleFactor() elsewhere. However, this is ImageBuffer's pixel ratio, not a device pixel ratio.

Yea, using deviceScaleFactor here was a dumb idea. I made a new patch that just cuts out using it altogether since it's misleading if the xScale and yScale differ by a little due to integer rounding. Also, I renamed the size of the ImageBuffer's internal backing store to be "internalSize".

[Matthew Delaney]

Created attachment 129086 [details]
Patch

[Matthew Delaney]

Committed r109016: <http://trac.webkit.org/changeset/109016>