Bug 227550 - [GPU Process] Draw PDFs using an intermediate ImageBuffer when using the GPU process
Summary: [GPU Process] Draw PDFs using an intermediate ImageBuffer when using the GPU ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-30 15:43 PDT by Myles C. Maxfield
Modified: 2021-07-08 13:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.95 KB, patch)
2021-06-30 15:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (9.20 KB, patch)
2021-07-06 15:26 PDT, Myles C. Maxfield
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-06-30 15:43:51 PDT
[GPU Process] Temporarily disable drawing large PDFs in display list drawing
Comment 1 Myles C. Maxfield 2021-06-30 15:48:46 PDT
Created attachment 432635 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-06-30 15:49:12 PDT
<rdar://problem/79989738>
Comment 3 Simon Fraser (smfr) 2021-06-30 16:18:35 PDT
Comment on attachment 432635 [details]
Patch

This will break some page, somewhere. Can we do something like draw at a lower resolution?
Comment 4 Myles C. Maxfield 2021-07-06 15:26:39 PDT
Created attachment 432982 [details]
Patch
Comment 5 Tim Horton 2021-07-08 11:22:15 PDT
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.
Comment 6 Myles C. Maxfield 2021-07-08 12:11:15 PDT
Committed r279741 (239522@main): <https://commits.webkit.org/239522@main>
Comment 7 Said Abou-Hallawa 2021-07-08 12:27:08 PDT
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?
Comment 8 Simon Fraser (smfr) 2021-07-08 13:28:54 PDT
The changelog used the old title, which is confusing.