Bug 227550 – [GPU Process] Draw PDFs using an intermediate ImageBuffer when using the GPU process

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[GPU Process] Temporarily disable drawing large PDFs in display list drawing

[Myles C. Maxfield]

Created attachment 432635 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79989738>

[Simon Fraser (smfr)]

Comment on attachment 432635 [details]
Patch

This will break some page, somewhere. Can we do something like draw at a lower resolution?

[Myles C. Maxfield]

Created attachment 432982 [details]
Patch

[Tim Horton]

Comment on attachment 432982 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:165
> +    // FIXME: Surely there needs to be a clip operation here, if the srcRect is smaller than the whole PDF.

BUG #

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:251
> +    // FIXME: Passing in m_cachedImageRect.location() for the source rect position is confusing here.

BUG # (also, "confusing" or "wrong"? I thought you had a test page that indicated the latter, which also should go in the BUG!)

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:286
> +            // We also need to be careful about not getting jetsam'ed.

I would expand "jetsam'ed" out to "terminated due to memory pressure" or something.

[Myles C. Maxfield]

Committed r279741 (239522@main): <https://commits.webkit.org/239522@main>

[Said Abou-Hallawa]

Comment on attachment 432982 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:165
>> +    // FIXME: Surely there needs to be a clip operation here, if the srcRect is smaller than the whole PDF.
> 
> BUG #

I do not think this comment is correct. The clipping has to be on context to dstRect regardless we are drawing the whole PDF or not. I think also the clipping is the responsibility of the caller, i.e. the render elements. BitmapImage::draw() does not clip either.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:291
> +            auto scalar = std::min(1.f, std::sqrt(static_cast<float>(s_maxCachedImageArea) / (dstRect.width() * dstRect.height())));

Can dstRect.width() or dstRect.height() be zero?

> LayoutTests/fast/images/pdf-in-canvas-expected.html:12
> +    <div style="position: absolute; left: -3px; top: 97px; width: 106px; height: 6px; background: green;"></div

Missing closing angle bracket.

> LayoutTests/fast/images/pdf-in-canvas.html:13
> +    <div style="position: absolute; left: -3px; top: 97px; width: 106px; height: 6px; background: green;"></div

Ditto.

> LayoutTests/fast/images/pdf-in-canvas.html:24
> +let image = document.getElementById("image");

You can create a detached image element instead. So you don't need to include the markup <img id="image"> and you do not need to set image.style.display = "none";

     var image = new Image();

> LayoutTests/fast/images/pdf-in-canvas.html:30
> +            context.fillRect(0, 0, 100, 100);

Why do we need to fill the canvas with black after drawing the image?

> LayoutTests/fast/images/pdf-in-canvas.html:32
> +            requestAnimationFrame(tick);

Why do we need to repeat this operation 10 times?

[Simon Fraser (smfr)]

The changelog used the old title, which is confusing.