When SurfacePipe::WritePixels() finishes writing early, zero out the rest of the surface

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
A lambda passed to SurfacePipe::WritePixels() can return WriteState::FINISHED, which marks the surface as complete and will forbid further writes even if the lambda hasn't yet written to every pixel. This is a feature that's only useful in exception circumstances, such as when image decoder input is truncated.

In this scenario it'd be better to zero out the rest of the surface. There are two reasons why:

(1) Doing this allows us to be sure that SurfaceFilters that buffer a number of rows have generated all of the output that they can. If we don't do this, we could end up in a situation where e.g. we output fewer rows of a truncated image when it's downscaled than when it's decoded at its intrinsic size.

(2) Even if we finish early, we should still generate an invalidation for the entire surface, and writing empty rows ensures that we'll do this.
(Assignee)

Comment 1

2 years ago
Created attachment 8762185 [details] [diff] [review]
When SurfacePipe::WritePixels() finishes early, zero out the rest of the surface.

Here's the patch. The additional testing tests more than just this change; I
wanted to make sure we had better coverage for exiting early from WritePixels()
in general.
Attachment #8762185 - Flags: review?(n.nethercote)
(Assignee)

Updated

2 years ago
Blocks: 1279117
Comment on attachment 8762185 [details] [diff] [review]
When SurfacePipe::WritePixels() finishes early, zero out the rest of the surface.

Review of attachment 8762185 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Would this have prevented bug 1217465?
Attachment #8762185 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 3

2 years ago
Thanks for the review!

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Looks good.
> 
> Would this have prevented bug 1217465?

Not by itself, no, because that's a case where the writes to the final pixels/rows simply never come. SurfacePipe doesn't have a mechanism for detecting this right now. It *will* be able to detect it once it owns the surface it's writing to and the caller has to explicitly take possession of it; the plan is that when the caller takes the surface, we'll check if it's finished and run this exact code if it isn't.
(Assignee)

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/35530c11c1631b62e2e02c59e071f49d877b1184
Bug 1279617 - When SurfacePipe::WritePixels() finishes early, zero out the rest of the surface. r=njn

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/35530c11c163
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.