1311512 - Use recording draw target for mozPrintCallback canvases

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Tobias Schneider [:tobytailor]]

mozPrintCallback is used in PDF.js to invoke a function when a canvas is about to get printed, e.g. to re-render its content at a higher scale in hope to somehow match the printers resolution. Anyhow, depending on the platform, printing system and whether e10s is enabled (like in https://bugzilla.mozilla.org/show_bug.cgi?id=1310165), somewhere in the pipeline the content of those canvases get rasterized and printed at poor quality, since creating a sub canvas for a printed canvas leads to a drawImage call at some point, and from there its mostly up to the gfx backend whether to rasterize immediately or preserving used drawing commands between surfaces. A possible solution for this could be recording all drawing commands ourselfs and replaying them on the target surface, treading it like inlined content to prevent rasterization when composing sub surfaces.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: bug1311512.diff

Proof of concept, already improves printing with PDF.js massively.

Comment 2

[Tobias Schneider [:tobytailor]]

Attachment: Use recording draw target for mozPrintCallback canvases

Landing candidate.

Comment 3

[Tobias Schneider [:tobytailor]]

Comment on attachment 8806913 [details] [diff] [review]
Use recording draw target for mozPrintCallback canvases

Jonathan, would like you to have a first look at this. Please keep in mind that this only affects printing canvases with mozPrintCallback defined on them, atm only used by PDF.js. Create PDF to test this and compare to FF without this patch, is https://www.irs.gov/pub/irs-pdf/f1040.pdf.

Comment 4

[Jonathan Watt [:jwatt]]

I do wonder whether we can't fix this by just making sure that intermediary DrawTargets and SourceSurfaces are of the same type. For example, the patch I've attached to bug 1310165 helps move us in that direction.

Your approach here seems good if we're sure that we do have cases of gfx backends rasterizing and we're sure that's not down to us using the wrong backends in places. I'd feel more comfortable with specific steps to reproduce an issue where we know that's the case.

That said, I'm not sure about the changes to BasicCanvasLayer.cpp in your patch. It seems like they at least ignore aMaskLayer, and it really seems like the code for replaying a SurfaceType::RECORDING surface onto a DrawTarget should be in the DrawTarget implementations, not scattered around wherever we think we might encounter such a surface. I need to think about this some more.

Comment 5

[Jonathan Watt [:jwatt]]

Tobias, can you comment on the third paragraph in the previous comment? More specifically it seems your new code path will cause rendering problem in certain scenarios by failing to apply aDeviceOffset and mBounds (rendering could be incorrectly offset or bounded), mSamplingFilter, GetEffectiveOpacity() (group opacity will be ignored), GetEffectiveOperator() (the wrong compositing operator could be used) and aMaskLayer (masking will be ignored). 

I think to avoid problems like that it would be necessary to push the SourceSurfaceRecording handling code all the way through FillRectWithMask back to the DrawTargetRecording methods that handle the SourceSurface (e.g. DrawTargetRecording::MaskSurface).

Can you also provide a PDF that still has problems when printed via PDF.js in a current Nightly build (that is, after all the recent printing fixes). I'd like to take a look at a concrete case.

Comment 6

[Tobias Schneider [:tobytailor]]

Tested with latest Nightly. Issues still exist. See https://www.irs.gov/pub/irs-pdf/f1040.pdf for example.

Comment 7

[Tobias Schneider [:tobytailor]]

> I think to avoid problems like that it would be necessary to push the
> SourceSurfaceRecording handling code all the way through FillRectWithMask
> back to the DrawTargetRecording methods that handle the SourceSurface (e.g.
> DrawTargetRecording::MaskSurface).

I agree that we should move the logic to address mentioned issues. I wouldn't go all the way through FillRect in the DrawTarget classes since this would require implementations for each backend. FillRectWithMask seems to be a good place for this. Would you agree?

Comment 8

[Jet Villegas (inactive)]

(In reply to Tobias Schneider [:tobytailor] from comment #7)
> > I think to avoid problems like that it would be necessary to push the
> > SourceSurfaceRecording handling code all the way through FillRectWithMask
> > back to the DrawTargetRecording methods that handle the SourceSurface (e.g.
> > DrawTargetRecording::MaskSurface).
> 
> I agree that we should move the logic to address mentioned issues. I
> wouldn't go all the way through FillRect in the DrawTarget classes since
> this would require implementations for each backend. FillRectWithMask seems
> to be a good place for this. Would you agree?

If it works, ship it :-)

Seems easy enough to put it in FillRectWithMask, then move it further down the stack if needed.

Comment 9

[Tobias Schneider [:tobytailor]]

Attachment: Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

Review commit: https://reviewboard.mozilla.org/r/100402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100402/

Comment 10

[Tobias Schneider [:tobytailor]]

Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100402/diff/1-2/

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

https://reviewboard.mozilla.org/r/100402/#review101006

::: gfx/layers/basic/BasicCanvasLayer.cpp:113
(Diff revision 2)
> -                        PreTranslate(0.0f, mBounds.height).
> +                      PreTranslate(0.0f, mBounds.height).
> -                        PreScale(1.0f, -1.0f));
> +                      PreScale(1.0f, -1.0f));

I'd rather you drop these indentation changes from BasicCanvasLayer.cpp (especially as you're no longer otherwise touching this file). Your new indentation makes it look like there are three arguments being passed to SetTransform. I personally find the previous indentation clearer in that it draws attention to the fact that what we have is a chain of calls.

::: gfx/layers/basic/BasicLayersImpl.cpp:126
(Diff revision 2)
>                   const DrawOptions& aOptions,
>                   ExtendMode aExtendMode,
>                   SourceSurface* aMaskSource,
>                   const Matrix* aMaskTransform,
>                   const Matrix* aSurfaceTransform)
>  {

At the top of this function can you add the following assert to make our current invariant clear:

MOZ_ASSERT((aMaskSource && aMaskTransform) ||
           (!aMaskSource && !aMaskTransform),
           "Either both or neither must be specified");

::: gfx/layers/basic/BasicLayersImpl.cpp:149
(Diff revision 2)
> +    aDT->SetTransform(oldTransform);
> +    aDT->PopClip();
> +    return;
> +  }
> +
> +  if (aSurface->GetType() == SurfaceType::RECORDING) {

At the top of this |if| block can you add:

MOZ_ASSERT(aOptions.mAlpha == 1.0 &&
           aOptions.mCompositionOp == CompositionOp.OP_OVER);

As I've mentioned in comment 5, I think this assert will fail on some PDF content. I'll attach such a PDF shortly.

::: gfx/layers/basic/BasicLayersImpl.cpp:158
(Diff revision 2)
> +    Matrix transform = oldTransform;
> +    if (aSurfaceTransform) {
> +      transform = (*aSurfaceTransform) * transform;
> +    }
> +
> +    InlineTranslator* translator = new InlineTranslator(aDT, transform); 

Nit: you have some trailing white space here.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: PDF containting group transparency

Comment 13

[Jonathan Watt [:jwatt]]

FWIW Lee and I fixed bug 1322729 which again pushes us closer to not needing this hack. Worth retesting if it's still needed before landing.

Comment 14

[Tobias Schneider [:tobytailor]]

Just tested it, it is still needed :).

Comment 15

[Tobias Schneider [:tobytailor]]

Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100402/diff/2-3/

Comment 16

[Carsten Book [:Tomcat]]

mozreview lists 2 issues, can this be fixed so that we can use autoland ? thanks

Comment 17

[Tobias Schneider [:tobytailor]]

Marked them as fixed.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: POC of possible alternative approach

(In reply to Tobias Schneider [:tobytailor] from comment #6)
> Tested with latest Nightly. Issues still exist. See
> https://www.irs.gov/pub/irs-pdf/f1040.pdf for example.

Okay, I finally got some time to dig in and debug this. All the following refers to the content process.

PrintTargetThebes creates and caches a non-recording mRefDT (non-recording because nsDeviceContextSpecProxy hasn't created its mRecorder yet):

  PrintTargetThebes::GetReferenceDrawTarget
  nsDeviceContext::CreateRenderingContextCommon
  nsDeviceContext::CreateReferenceRenderingContext
  mozilla::PresShell::CreateReferenceRenderingContext
  mozilla::PresShell::DoReflow
  mozilla::PresShell::ProcessReflowCommands
  mozilla::PresShell::FlushPendingNotifications
  mozilla::PresShell::FlushPendingNotifications
  nsPrintEngine::ReflowPrintObject
  nsPrintEngine::ReflowDocList
  nsPrintEngine::InitPrintDocConstruction
  nsPrintEngine::DoCommonPrint
  nsPrintEngine::CommonPrint
  nsPrintEngine::Print
  nsDocumentViewer::Print

nsDeviceContextSpecProxy creates its mRecorder:

  nsDeviceContextSpecProxy::BeginDocument
  nsDeviceContext::BeginDocument
  nsPrintEngine::SetupToPrintContent
  nsPrintEngine::DocumentReadyForPrinting
  nsPrintEngine::AfterNetworkPrint
  nsPrintEngine::InitPrintDocConstruction
  nsPrintEngine::DoCommonPrint
  nsPrintEngine::CommonPrint
  nsPrintEngine::Print
  nsDocumentViewer::Print

When asked for a reference DT, PrintTargetThebes returns the non-recording cached mRefDT even though it is now being passed aRecorder: 

  PrintTargetThebes::GetReferenceDrawTarget
  nsDeviceContext::CreateRenderingContextCommon
  nsDeviceContext::CreateReferenceRenderingContext
  nsSimplePageSequenceFrame::PrePrintNextPage
  nsPrintEngine::PrePrintPage
  nsPagePrintTimer::Notify

So basically we're not recording the draw commands to any of the DTs created by calling CreateSimilarDrawTarget on the ref DT, such as the DTs used drawn to by mozPrintCallback. As a result we end up Snapshot'ing and passing that snapshot over to the chrome process to composite.

This POC patch fixes the text in f1040.pdf so that it is still text. I've been working on killing PrintTargetThebes though, so if we were to eventually back out your fix for this bug in favor of something like this the eventual patch would be something different.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please ensure that the patch has the proper reviews set in MozReview so it can autoland.

Comment 20

[Tobias Schneider [:tobytailor]]

Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

https://reviewboard.mozilla.org/r/100402/#review101632

Comment 21

[Tobias Schneider [:tobytailor]]

Not quite sure what "proper reviews set" means and what to do here. Can someone help out?

Comment 22

[Jonathan Watt [:jwatt]]

Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

https://reviewboard.mozilla.org/r/100402/#review103570

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c6803c30303
Use recording draw target for mozPrintCallback canvases. r=jwatt

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for mass asserting:

https://hg.mozilla.org/integration/autoland/rev/febb7f7551e2abfa720e14f39c6f48e3884648ea

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5c6803c30303f93393b68e5563e8b467b7d5984a
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=66910322&repo=autoland
> Assertion failure: (aMaskSource && aMaskTransform) || (!aMaskSource && !aMaskTransform) (Either both or neither must be specified), at /home/worker/workspace/build/src/gfx/layers/basic/BasicLayersImpl.cpp:129

Comment 25

[Jonathan Watt [:jwatt]]

Meh. It looks like BasicCompositor::DrawQuad calls FillRectWithMask passing a mask transform whether or not it passes the a mask. Okay, let's do the simple thing and change the assertion to assert that if aMaskSurface is passed that the calling code also passes aMaskTransform.

Comment 26

[Jonathan Watt [:jwatt]]

Here's a Try push with that change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c84df5f61137d4fb923963a2b5f59e28470e08

Comment 27

[Tobias Schneider [:tobytailor]]

Can we land it with that change?

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4849ef8c9a34
Use recording draw target for mozPrintCallback canvases. r=jwatt

Comment 29

[Jonathan Watt [:jwatt]]

Try looks ok to me, so I went ahead and did that for you.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4849ef8c9a34

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

https://reviewboard.mozilla.org/r/100402/#review139410

::: gfx/2d/InlineTranslator.h:39
(Diff revision 3)
> +
> +  bool TranslateRecording(std::istream& aRecording);
> +
> +  DrawTarget* LookupDrawTarget(ReferencePtr aRefPtr) final
> +  {
> +    return mBaseDT;

Just returning mBaseDT here is wrong