fix DrawTargetSkia to not copy and render in place

RESOLVED FIXED in mozilla28

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
mozilla28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
DrawTargetSkia copies the pixel data it is given when using InitWithData and then draws into the copy, ultimately losing it when the DT is deallocated. Instead, attach the bitmap to the pixel data and draw into that. The pixel data must stick around or this fails. I think InitWithData is meant this way.

Dito for SourceSourceSkia. It currently allocates a separate bitmap and copies the data in there. Thats very inefficient and I think not designed this way in Moz2D.
(Assignee)

Comment 1

4 years ago
Created attachment 820107 [details] [diff] [review]
Don't allocate pixel data for DrawTargetSkia and SourceSurfaceSkia when using InitWithData.
Attachment #820107 - Flags: review?(gwright)
Comment on attachment 820107 [details] [diff] [review]
Don't allocate pixel data for DrawTargetSkia and SourceSurfaceSkia when using InitWithData.

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +755,1 @@
>    mSize = aSize;

bitmap never gets given the aData pointer here?

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +77,5 @@
>      mBitmap.unlockPixels();
>      mBitmap.notifyPixelsChanged();
>      mBitmap.setIsOpaque(true);
> +  } else {
> +    mBitmap = temp;

We can't do this because temp's pixels are currently set to aData, and CreateSourceSurfaceFromData (which in turns calls InitFromData) explicitly doesn't take ownership of aData.
Attachment #820107 - Flags: review?(gwright) → review-
(Assignee)

Updated

4 years ago
Summary: fix DrawTargetSkia and SourceSurfaceSkia → fix DrawTargetSkia to not copy and render in place
(Assignee)

Comment 3

4 years ago
Created attachment 825239 [details] [diff] [review]
Make DrawTargetSkia render into the memory buffer provided. The caller is responsible to make sure the memory isn't freed until Flush() is called.
Assignee: nobody → gal
Attachment #820107 - Attachment is obsolete: true
Attachment #825239 - Flags: review?(gwright)
(Assignee)

Comment 4

4 years ago
On a related note, skia silently fails to render RGB in various exciting ways, only BGR is handled properly in most paths. Is that intentional?
(Assignee)

Updated

4 years ago
Blocks: 933030
Attachment #825239 - Flags: review?(gwright) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d642c4a393c
Keywords: checkin-needed

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/4d642c4a393c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.