Add helpers to snapshot DTs for a specific format and pre- and unpremultiplication

NEW
Assigned to

Status

()

Core
Graphics
4 years ago
4 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
I recently added some code to convert BGRX->BGRA (to eliminate thebes from Canvas2D). Such manual conversion seems suboptimal because it doesn't leverage any backend smarts we have. In particular we could do the BGRX->BGRA conversion on the GPU, along with pre- and unpremultiplication (which WebGL needs).

I am handing in a draw target so we can snapshot it or create similar draw targets and blit onto those for format conversion.
(Assignee)

Comment 1

4 years ago
Created attachment 826549 [details] [diff] [review]
Part 1: Add helpers to snapshot for specific formats and pre-/unpremultiply.

The ugly table will be eliminated in a couple days once mstange's FILTER code lands.
Assignee: nobody → gal
(Assignee)

Comment 2

4 years ago
Created attachment 826550 [details] [diff] [review]
Part 2: Use new code path for Canvas2D.
(Assignee)

Comment 3

4 years ago
Created attachment 826552 [details] [diff] [review]
Part 3: Avoid creating a thebes context in WebGLContext.cpp.
(Assignee)

Comment 4

4 years ago
Created attachment 826554 [details] [diff] [review]
Part 3: Avoid creating a thebes context in WebGLContext.cpp.
Attachment #826552 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #826549 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #826550 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #826554 - Flags: review?(matt.woodrow)
Comment on attachment 826549 [details] [diff] [review]
Part 1: Add helpers to snapshot for specific formats and pre-/unpremultiply.

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +781,5 @@
>  
>    SkBitmap bitmap;
>    bitmap.setConfig(GfxFormatToSkiaConfig(aFormat), aSize.width, aSize.height, aStride);
>    bitmap.setPixels(aData);
> +  bitmap.setIsOpaque(aFormat == FORMAT_B8G8R8X8 || aFormat == FORMAT_R5G6B5);

isOpaque seems unnecessary now.

::: gfx/2d/SourceSurfaceHelpers.cpp
@@ +44,5 @@
> +  }
> +
> +  Premultiply(data->GetData(), data->GetSize(), data->Stride());
> +
> +  data->MarkDirty();

We've never actually used this anywhere, and none of our backends actually implement it.

For D2D at least it won't work if you try draw with this surface (since we do a read-only lock of the data).

I realize you're not actually drawing this snapshot anywhere, but it seems like abuse of the API to return a snapshot in a broken state (where the GetData() returns different values to what would be drawn).

Can we instead not expose these functions, and make the premultiply/unpremlt an option to GetPackedPixelData. That way we never expose the broken snapshot data, and it's just a temporary state before we copy to the new data. Maybe even do the conversion while copying.
Attachment #826550 - Flags: review?(matt.woodrow) → review+
Attachment #826549 - Flags: review?(matt.woodrow) → review+
Attachment #826554 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 6

4 years ago
We will do the premultiplication with mstange's filters once they land. mstange when does your code land?
Flags: needinfo?(mstange)
It has landed. (bug 924102)
Flags: needinfo?(mstange)

Comment 8

4 years ago
is this still needed ?
You need to log in before you can comment on or make changes to this bug.