Closed Bug 1039568 Opened 6 years ago Closed 5 years ago

Add a capture DrawTarget to Moz2D

Categories

(Core :: Graphics, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(2 files)

For tiling we'd like the ability to capture a series of drawing commands and replay them in a consistent fashion across multiple tiles. This patch aims to add that support.
Attachment #8456945 - Flags: review?(jmuizelaar)
Comment on attachment 8456945 [details] [diff] [review]
Add DrawTargetCapture to Moz2D

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

::: DrawTargetCapture.h
@@ +138,5 @@
> +
> +  // This storage system was used to minimize the amount of heap allocations
> +  // that are required while recording.
> +  template<typename T>
> +  void AppendToCommandList(const T& aDrawingCommand)

We could avoid a copy by returning the pointer and then constructing in-place from the caller. Unless the compiler can optimize to that from this code, but I'm not convinced that it can.

@@ +150,5 @@
> +  RefPtr<DrawTarget> mRefDT;
> +
> +  IntSize mSize;
> +
> +  std::vector<uint8_t> mDrawCommandStorage;

Won't we leak everything in here when it's destructed?

It seems like we need to iterate over the area again (possibly combined with the final replay) and invoke the destructors.

Or we could store raw pointers to things in mDrawCommandDrawStorage and keep an array of RefPtr<SourceSurface> etc separately as lifetime management.

::: SourceSurfaceRawData.h
@@ +32,5 @@
>                          SurfaceFormat aFormat,
>                          bool aOwnData);
>  
> +  virtual void GuaranteePersistance();
> +

We could probably also add a method for users of this code to tell the SourceSurfaceRawData to take ownership of existing data. I expect in a lot of cases where we might hit the copy path we could use that instead. Not that easy to find where we need it other than just looking at profiles though.
Comment on attachment 8456945 [details] [diff] [review]
Add DrawTargetCapture to Moz2D

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

::: DrawTargetCapture.h
@@ +143,5 @@
> +  {
> +    size_t oldSize = mDrawCommandStorage.size();
> +    mDrawCommandStorage.resize(mDrawCommandStorage.size() + sizeof(T) + sizeof(uint32_t));
> +    uint8_t* nextDrawLocation = &mDrawCommandStorage.front() + oldSize;
> +    *(uint32_t*)(nextDrawLocation) = sizeof(T) + sizeof(uint32_t);

This can cause unaligned accesses. I'm not sure how much it matters (I suppose you could have a compiler that was using vector instructions and assuming alignment of particular things), but it's at least worth a comment, or just fix it to memcpy in and out of the buffer. The sizes are small enough that I don't think the copies will matter... Up to you.
Attachment #8456945 - Flags: review?(jmuizelaar) → review-
Attachment #8456945 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/150cdebe837f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1055968
You need to log in before you can comment on or make changes to this bug.