Closed Bug 1311512 Opened 3 years ago Closed 3 years ago

Use recording draw target for mozPrintCallback canvases

Categories

(Core :: Printing: Output, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: tschneider, Assigned: tschneider)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch bug1311512.diff (obsolete) — Splinter Review
Proof of concept, already improves printing with PDF.js massively.
Landing candidate.
Attachment #8802675 - Attachment is obsolete: true
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.
Attachment #8806913 - Flags: review?(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.
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.
Flags: needinfo?(tschneider)
Tested with latest Nightly. Issues still exist. See https://www.irs.gov/pub/irs-pdf/f1040.pdf for example.
Flags: needinfo?(tschneider)
> 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?
Flags: needinfo?(jwatt)
(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.
Flags: needinfo?(jwatt)
Attachment #8806913 - Attachment is obsolete: true
Attachment #8806913 - Flags: review?(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.
Attachment #8821027 - Flags: review?(jwatt) → review+
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.
Just tested it, it is still needed :).
Keywords: checkin-needed
mozreview lists 2 issues, can this be fixed so that we can use autoland ? thanks
Flags: needinfo?(tschneider)
Marked them as fixed.
Flags: needinfo?(tschneider)
Keywords: checkin-needed
(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.
Please ensure that the patch has the proper reviews set in MozReview so it can autoland.
Keywords: checkin-needed
Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

https://reviewboard.mozilla.org/r/100402/#review101632
Attachment #8821027 - Flags: review+
Not quite sure what "proper reviews set" means and what to do here. Can someone help out?
Flags: needinfo?(ryanvm)
Flags: needinfo?(jwatt)
Comment on attachment 8821027 [details]
Bug 1311512 - Use recording draw target for mozPrintCallback canvases.

https://reviewboard.mozilla.org/r/100402/#review103570
Assignee: nobody → tschneider
Flags: needinfo?(ryanvm)
Flags: needinfo?(jwatt)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5c6803c30303
Use recording draw target for mozPrintCallback canvases. r=jwatt
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
Flags: needinfo?(tschneider)
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.
Can we land it with that change?
Flags: needinfo?(tschneider)
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4849ef8c9a34
Use recording draw target for mozPrintCallback canvases. r=jwatt
Try looks ok to me, so I went ahead and did that for you.
https://hg.mozilla.org/mozilla-central/rev/4849ef8c9a34
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1345815
Depends on: 1347646
Depends on: 1347632
Blocks: 1347626
Blocks: 1349616
Depends on: 1350738
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
You need to log in before you can comment on or make changes to this bug.