web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

RFC #41: print #41

Closed gsnedders closed 4 years ago

gsnedders commented 4 years ago

This has been long discussed, but to put a concrete proposal on the table to resolve https://github.com/web-platform-tests/wpt/issues/7760. As outlined in the RFC, this is largely motivated by unblocking https://github.com/web-platform-tests/wpt/issues/5381.

I know @jgraham coincidentally spoke to @jwatt earlier today about requirements here, and I swear I had no idea when I started writing this.

Also refs https://github.com/w3c/webdriver/issues/1093.

Hexcles commented 4 years ago

A question: how and where are we going to do the rasterization and diffing of PDF? Does that mean we need a PDF rendering engine in wptrunner?

gsnedders commented 4 years ago

@Hexcles the proposal was to call Ghostscript from wptrunner, so Ghostscript becomes a dependency

foolip commented 4 years ago

Two weeks have passed on this. @stephenmcgruer is this functionality that you think we'd use in Blink? Would a Ghostscript dependency be acceptable?

@gsnedders would you also work on implementing this?

stephenmcgruer commented 4 years ago

At this point, we have not evaluated whether Ghostscript would be an acceptable dependency for Chromium infra. There does not seem to be a strong impetus behind this RFC currently, nor am I aware of any strong desire from Blink, so this is very low on our list of priorities to look at.

If someone wishes to actively champion this RFC and its subsequent implementation, we would then be happy to do the legwork to determine if we could take that dependency.

jgraham commented 4 years ago

I'm actually working on this for Firefox. I agree the ghostscript thing is problematic; it seems like the Python bindings in this space basically rely on system ghostscript, so it doesn't end up looking like a compiled dependency in our code, but does depend on having the right system packages installed.

In practice I expect the Firefox implementation to end up skipping the actual PDF rendering in most cases and just have an internal API that renders in paginated mode to a set of canvases, and does the pixel comparison based on that. But we're a little way away from that point.

jgraham commented 4 years ago

OK, so I looked some more at Ghostscript and I think the fact that it's AGPL rules it out.

However I have a somewhat working patch that vendors PDFjs into the tree and first gets a PDF out of the browser and then does execute_async_script in a window with PDFjs loaded to render the PDF data to PNGs. PDFjs is Apache licensed, so I think that's going to be a much more acceptable dependency, and it also allows us to vendor all the things, which makes it much easier to deploy this to infra.

Hexcles commented 4 years ago

@jgraham thanks for the explanation.

Licensing is a good point. And you're right; we can't even touch Ghostscript. I like the idea of using PDFjs.

Now that we start to discuss the actual implementation of "printing to PDF", do we want to make that part of this RFC? My understanding of the RFC as it's written is to "agree what we're doing with print tests" without going into details. To be clear, I think it's fine to omit the details and even have Gecko use some internal API for now, but it'd be great if we can have a path to standardization (i.e. "print to PDF" in WebDriver) in mind.

jgraham commented 4 years ago

I think to accept this RFC we need to be convinced that the feature is possible to implement in terms of standards*. So enough implementation details are necessary to demonstrate that. For WebDriver I already made a PR to the spec that adds a print endpoint https://github.com/w3c/webdriver/pull/1468 So I'm going to make an initial implementation that works in terms of that.

* Strictly speaking it isn't because PDF. But I think it's OK to gloss that if we can find an implementation that we're happy everyone can use, and we accept whatever risk comes from using a different implementation.

jgraham commented 4 years ago

https://github.com/web-platform-tests/wpt/pull/21901 is my very WIP implementation. Currently that requires that print reftests have -print in the name, although we could switch to the gecko-like thing of them having a specific class on the body.

The implementation is via WebDriver print to PDF and then uses PDFjs to render the PDF to a set of PNGs. The page size is set to 5x3 inches, and all margins are 0. The integration with logging is basically terrible (the log formats assume one PNG for the test and one for the ref, so we currently output the last one). Also there's likely a bunch of brokenness for mismatch.

It currently passes a single infrastructure test that I somewhat fine-tuned to work in gecko (I tried to create the reference using a div the height of the page which I expected to be 3 inches / 288px but turned out to be 280px for reasons I don't fully understand).

To actually run this in Gecko you also need a gecko patch which removes the "unprintable margins" when generating PDFs from marionette.

@dbaron @jwatt idk if you're following this RFC, but perhaps you ought to be.

Hexcles commented 4 years ago

I think to accept this RFC we need to be convinced that the feature is possible to implement in terms of standards*. So enough implementation details are necessary to demonstrate that. For WebDriver I already made a PR to the spec that adds a print endpoint w3c/webdriver#1468 So I'm going to make an initial implementation that works in terms of that.

Wonderful! To clarify, your prototype in https://github.com/web-platform-tests/wpt/pull/21901 uses Marionette, not the new proposed WebDriver command, right?

jgraham commented 4 years ago

Wonderful! To clarify, your prototype in web-platform-tests/wpt#21901 uses Marionette, not the new proposed WebDriver command, right?

Yes, but the difference is very small; basically the marionette command is an implementation of the proposed WebDriver command, the only difference is that the transport layer isn't HTTP (the HTTP<->marionette conversion is basically what GeckoDriver does).

gsnedders commented 4 years ago

I have some concerns around using pdf.js (primarily because especially if we end up with any page with a lot of paths in the PDF it becomes rather slow). I don't think we need to define in the RFC how we're going to actually achieve it beyond "compare the PDFs when rasterised pixel-for-pixel", I think we just need some proof of implementability in terms of standards.

Poppler is another option when it comes to PDF software (GPLv2/3).

jgraham commented 4 years ago

I think the primary motivating factor for pdf.js is that we can distribute it in the repo without adding additional system dependencies or build steps. As a baseline implementation that overrides any concerns about performance, although I suspect that vendors may end up with different tradeoffs in their CI. Also, FWIW the existing gecko print reftests seem to be using PDF.js for basically the same reason (i.e. it's already in tree).

jgraham commented 4 years ago

I'm not aware that gecko ever had PDF-based print reftests; I think it had layout-as-paginated-and-render-to-a-canvas because that's faster (but doesn't test the whole print pipeline) and print-to-pdf as a seperate test type specifically for bugs around not keeping text as text in output PDFs.

I don't know the historical situation with WebKit/Blink as well.

Whatever mechanism we have here has to have some basis in standards in order to make it theoretically viable. WebDriver has the ability to get a paginated layout as a PDF so that's a viable thing we could build on. Otherwise we'd need some custom test API to do paginated layout and that would end up being more work. Having said that I would consider it a totally reasonable implementation strategy to skip the actual PDF generation and just perform the comparison against internal graphics buffers, as long as they're laid out for paginated representation at the correct dimensions. This is something we might well do in gecko at some point for performance reasons.

Having said that, the demo implementation doesn't seem to have lots of issues with unexpected mismatches (based on really early results). So it's possible that this was worse in other browsers or worse because of some implementation details (e.g. skipping the rasterization step and just trying to compare the PDFs directly). So I think your concern is legitimate, but I don't think it should block the work. In the worst case if this turns out to not be good enough we can use it as a reason to expose a render-to-canvases-as-paginated (or something) method to tests later, and build the functionality on top of that.

stephenmcgruer commented 4 years ago

WebDriver has the ability to get a paginated layout as a PDF so that's a viable thing we could build on

I mean yes, no disagreement, but didn't you add that to the webdriver standard for this RFC? So there's nothing to say 'print to PNG' couldn't be a webdriver standard too.

I don't think it should block the work. In the worst case if this turns out to not be good enough we can use it as a reason to expose a render-to-canvases-as-paginated (or something) method to tests later, and build the functionality on top of that.

There is a cost to landing an RFC if it ultimately can't be supported. Every test written is accumulated debt if we ever have to change our minds, and there is also a 'trust' cost when people have to unlearn an old API and learn a new one (I've had to deal with that over assert_precondition).

That said, I think the onus here is definitely on me to take the next step, so let me reach out to Chromium folks and see if I find someone who might be able to comment on our ability to consistently render PDFs. If I can't get anything conclusive from that, then we should just land this and take on that burden. I understand and support the higher level goal here (testing paginated content).

jgraham commented 4 years ago

I mean yes, no disagreement, but didn't you add that to the webdriver standard for this RFC? So there's nothing to say 'print to PNG' couldn't be a webdriver standard too.

I don't think that's entirely fair. I mostly added it to WebDriver because we've seen a lot of people using things like Puppeteer just for render-to-PDF. Being able to do that in a standard way seems obviously useful since it's meeting real use cases. Being able to use it for pagniation tests was an added bonus.

We could add print-to-PNG but that's harder to justify in terms of non-webdriver uses.

There is a cost to landing an RFC if it ultimately can't be supported. Every test written is accumulated debt if we ever have to change our minds, and there is also a 'trust' cost when people have to unlearn an old API and learn a new one (I've had to deal with that over assert_precondition).

I don't think it's going to matter from a test author point of view what the internal implementation looks like. The user-facing API will be "you make two documents that when rendered as paged content on a particular paper size compare equal for all their pages (or as defined in the reftest-pages meta element". That's why I think moving away from a PDF intermediate can even be an optimistation rather than anything that requires cross-vendor buy in.

stephenmcgruer commented 4 years ago

I don't think that's entirely fair. I mostly added it to WebDriver because we've seen a lot of people using things like Puppeteer just for render-to-PDF. Being able to do that in a standard way seems obviously useful since it's meeting real use cases. Being able to use it for pagniation tests was an added bonus.

That's fair, I apologise for not accurately representing that situation. I do think it is not fair to say that print to PNG would necessarily be a test-only API though; we haven't explored whether web developers would also want to print to PNG or not.

I don't think it's going to matter from a test author point of view what the internal implementation looks like. ...

I think I'm persuaded here in general. I was thinking on it and even if a certain platform renders content differently than others, it should hopefully render the ref content in a similar way (otherwise either the test ref is bad, or the browser has a nasty surprise for users...).

The big risk seems to be if a PDF approach doesn't work at all (for whatever reason), and then we need to find some other way to do the cross-browser implementation. There are paths forward there (spec a print-to-PNG command, spec a command that equates to testRunner.setPrinting(), etc), so I think that risk is manageable.

I have some editorial comments on the RFC, but am out of time today to do it - will do that review tomorrow.

stephenmcgruer commented 4 years ago

Thanks for addressing the comments @jgraham . It's been 10 days since my approval with nobody else bringing up disagreements, so I'd be happy to see this merged.

hiikezoe commented 4 years ago

\o/

jgraham commented 4 years ago

PR for the implementation at https://github.com/web-platform-tests/wpt/pull/21901 (@hiikezoe I'll file a phab patch with the gecko-internal bits once that's landed; it's better to get review of the wpt changes here)

gsnedders commented 4 years ago

There appears to have been difficulty on the WebKit/Blink side in making consistent PDFs (unclear if this is beyond what fuzzy diffing could handle), whilst the reason for Gecko doing so is unclear.

AIUI, WebKit was doing byte-for-byte PDF comparisons and very quickly found out these always differed between supported platforms (so you'd need per-platform expectations for every test). Note that back then WebKit was yet to add support for -expected.html: the standard options were -expected.txt (a weird innerText based dump) or -expected.png (an expected screenshot, which gives a very big incentive to avoid text to make it the same cross-platform).