Closed Bug 1331938 Opened 3 years ago Closed 3 years ago

Refactor imagelib in preparation for WebRender

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

Details

Attachments

(5 files, 14 obsolete files)

1.37 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
7.52 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
18.14 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
4.74 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
24.04 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
This bug tracks imagelib refactoring which facilitates switching over to WebRender, without including any WebRender specific changes.
Assignee: nobody → aosmond
Blocks: 1331944
Attachment #8827903 - Attachment is obsolete: true
Attachment #8827989 - Attachment is obsolete: true
Attachment #8827987 - Attachment is obsolete: true
Attachment #8828867 - Flags: review?(nical.bugzilla)
Attachment #8827902 - Flags: review?(tnikkel)
Attachment #8828052 - Flags: review?(tnikkel)
Comment on attachment 8828867 [details] [diff] [review]
Part 2. Add SourceSurfaceSharedData, v3

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

R=me if you move the SourceSurfaceSharedData class (and more generally anything that depends on more than the standard library or mfbt) out of the gfx/2d directory which we want to be able to build separately without the extra dependencies.
Attachment #8828867 - Flags: review?(nical.bugzilla) → review+
Move to gfx/layers as per feedback in part 2.
Attachment #8827902 - Attachment is obsolete: true
Attachment #8827902 - Flags: review?(tnikkel)
Attachment #8829434 - Flags: review?(tnikkel)
Move to gfx/layers as per review feedback.
Attachment #8828867 - Attachment is obsolete: true
Attachment #8829435 - Flags: review+
Update as per changes in part 1.
Attachment #8828052 - Attachment is obsolete: true
Attachment #8828052 - Flags: review?(tnikkel)
Attachment #8829436 - Flags: review?(tnikkel)
Comment on attachment 8829434 [details] [diff] [review]
Part 1. Add SourceSurfaceVolatileData, v2

Seems reasonable. A gfx/ peer should also review.

>+/**
>+ * This class is used to wrap volatile data buffers used for source surfaces.
>+ * The Map and Unmap semantics are used to guarantee that the volatile data
>+ * buffer is not freed by the operating system while the surface is in active
>+ * use. If GetData is expected to return a non-null value without a
>+ * corresponding Map call (and verification of the result), the surface data
>+ * should be wrapped in a temporary SourceSurfaceRawData with a ScopedMap
>+ * closure.
>+ */

Instead of having a raw GetData function and hoping the callers properly map/unmap can we enforce these semantics with a RAII class which is the only way to access the underlying data that handles the mapping/unmapping?

>+  // Althought Map (and Moz2D in general) isn't normally threadsafe,
>+  // we want to allow it for SourceSurfaceVolatileData since it should
>+  // always be fine (for reading at least).

Can we enforce that at all (the read-only part)?
Attachment #8829434 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> Comment on attachment 8829434 [details] [diff] [review]
> Part 1. Add SourceSurfaceVolatileData, v2
> 
> Seems reasonable. A gfx/ peer should also review.
> 
> >+/**
> >+ * This class is used to wrap volatile data buffers used for source surfaces.
> >+ * The Map and Unmap semantics are used to guarantee that the volatile data
> >+ * buffer is not freed by the operating system while the surface is in active
> >+ * use. If GetData is expected to return a non-null value without a
> >+ * corresponding Map call (and verification of the result), the surface data
> >+ * should be wrapped in a temporary SourceSurfaceRawData with a ScopedMap
> >+ * closure.
> >+ */
> 
> Instead of having a raw GetData function and hoping the callers properly
> map/unmap can we enforce these semantics with a RAII class which is the only
> way to access the underlying data that handles the mapping/unmapping?
> 

There is DataSourceSurface::ScopedMap which is mostly used, but there are still a good number of places under gfx where DataSourceSurface::GetData is used. I think that would need to be a separate change, but I agree wholeheartedly :).

> >+  // Althought Map (and Moz2D in general) isn't normally threadsafe,
> >+  // we want to allow it for SourceSurfaceVolatileData since it should
> >+  // always be fine (for reading at least).
> 
> Can we enforce that at all (the read-only part)?

Hm, no. We come close with SourceSurfaceSharedBuffer, where we get actual memory pages that enables us to mark the page as read only when the decoding has completed. It might be possible to split DataSourceSurface::ScopedMap into DataSourceSurface::ScopedReadMap and DataSourceSurface::ScopedWriteMap -- the former could return a const uint8_t const * pointer which would at least make it obvious we aren't supposed to write.
Attachment #8829434 - Flags: review?(nical.bugzilla)
Attachment #8829434 - Flags: review?(nical.bugzilla) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/752fede74c5f
Part 1. Add SourceSurfaceVolatileData, a volatile data backed DataSourceSurface. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/53fee347291b
Part 2. Add SourceSurfaceSharedData, a shared data backed DataSourceSurface. r=nical
Keywords: leave-open
Backed out due to Windows build failures.
Backout by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12261233a2a5
Backed out changeset 53fee347291b r=backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f15e8d53f91
Backed out changeset 752fede74c5f r=backout
Rebase on inbound.
Attachment #8829434 - Attachment is obsolete: true
Attachment #8831131 - Attachment description: Part 1. Add SourceSurfaceVolatileData, v2.1 → Part 1. Add SourceSurfaceVolatileData, v2.1 [carries r=nical]
Attachment #8831131 - Flags: review+
Attachment #8829435 - Attachment is obsolete: true
Attachment #8831133 - Flags: review+
Same as attachment 8831216 [details] [diff] [review] but was produced with "hg qdiff --ignore-space-change" which clearly shows only a pair of scoping braces as added.
Status: NEW → ASSIGNED
Comment on attachment 8831216 [details] [diff] [review]
Part 2c. Fix rooting hazard in CanvasRenderingContext2D::GetImageDataArray

Looks like it passes try \o/.
Attachment #8831216 - Flags: review?(nical.bugzilla)
Comment on attachment 8829436 [details] [diff] [review]
Part 3. Switch to using SourceSurfaceVolatileData, v4

>     // We only need to memset it if the buffer was allocated on the heap.
>     // Otherwise, it's allocated via mmap and refers to a zeroed page and will
>     // be COW once it's written to.
>-    memset(vbufptr, 0, stride * aSize.height);
>+    size_t heapSize = 0;
>+    size_t nonHeapSize = 0;
>+    aSurface->AddSizeOfExcludingThis([] (const void *p) -> size_t {
>+      return 1;
>+    }, heapSize, nonHeapSize);
>+    if (heapSize) {
>+      memset(data, 0, stride * aSize.height);
>+    }
>   }

Using AddSizeOfExcludingThis is kinda hacky, can we just give it a function that returns if its on the heap?
Comment on attachment 8831216 [details] [diff] [review]
Part 2c. Fix rooting hazard in CanvasRenderingContext2D::GetImageDataArray

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +5651,5 @@
> +        }
> +        src += srcStride - (dstWriteRect.width * 4);
> +        dst += (aWidth * 4) - (dstWriteRect.width * 4);
> +      }
> +    } else

Let's use his as an occasion to properly brace the else block here.
Attachment #8831216 - Flags: review?(nical.bugzilla) → review+
Add DataSourceSurface::OnHeap API.
Attachment #8831131 - Attachment is obsolete: true
Attachment #8832027 - Flags: review+
Add SourceSurfaceSharedData::OnHeap override.
Attachment #8831133 - Attachment is obsolete: true
Attachment #8832029 - Flags: review+
Fix bracing.
Attachment #8831216 - Attachment is obsolete: true
Attachment #8831218 - Attachment is obsolete: true
Attachment #8832030 - Flags: review+
Rebase and use new OnHeap API.
Attachment #8829436 - Attachment is obsolete: true
Attachment #8829436 - Flags: review?(tnikkel)
Attachment #8832031 - Flags: review?(tnikkel)
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/faa107ec149b
Part 1. Add SourceSurfaceVolatileData, a volatile data backed DataSourceSurface. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/20978572be88
Part 2. Add SourceSurfaceSharedData, a shared data backed DataSourceSurface. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f324ee630b3
Part 2b. Fix missing includes causing unified build errors. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/52b841bf65ba
Part 2c. Fix rooting hazard in CanvasRenderingContext2D::GetImageDataArray. r=nical
Attachment #8832031 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc1fccf53d3
Part 3. Switch to using VolatileSourceSurface instead of VolatileBuffer directly in imgFrame. r=tnikkel
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/9fc1fccf53d3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1337790
Blocks: 1074192
You need to log in before you can comment on or make changes to this bug.