1187812 - [Foxeye] Let WebGL::TexImageSource accept ImageBitmap as a source object

Status: RESOLVED DUPLICATE of bug 1324924

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[u459114]

This bug is to let TexImage2D accept a BitmapImage as source object. 

Before this change, to draw a BitmapImage into a 3D canvas, you need to 
1. Fetch buffer data from that BitmapImage.
 [1] bitmap.mapDataInto(RGBA32, buffer, long offset);
2. Feed that buffer to a ImageData object
 var data = new ImageData();
 data.data = buffer;
3. Upload this image data via TexImage2D
 [2] ctx.texImage2D(..., data);

After this change, we need only one step
 ctx.texImage2D(..., bitmap);

On B2G, if the internal buffer of a BitmapImage is a graphic buffer, we may prevent texture upload by using eglImage binding instead.

[1] BitmapImage extension: http://kakukogou.github.io/spec-imagebitmap-extension/#idl-def-ImageBitmap
[2] WebGL: https://www.khronos.org/registry/webgl/specs/latest/1.0/

Comment 1

[u459114]

Attachment: WIP - Accept ImageBitmap as source buffer of TexImage2D

Comment 2

[u459114]

Attachment: WIP - Accept ImageBitmap as source buffer of TexImage2D

Comment 3

[u459114]

Attachment: WIP - Accept ImageBitmap as source buffer of TexImage2D

Comment 4

[u459114]

Attachment: Testing sample

Comment 5

[Tzuhao Kuo [:kaku]]

It seems that bug#1141979 does not block this bug, right?

Comment 6

[u459114]

Reference
https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLBlitHelper.cpp#1015
https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLBlitHelper.cpp#761

Comment 7

[u459114]

Attachment: WIP - Accept ImageBitmap as source buffer of TexImage2D

Comment 8

[u459114]

Comment on attachment 8652163 [details] [diff] [review]
WIP - Accept ImageBitmap as source buffer of TexImage2D

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

Any suggestion?

Comment 9

[u459114]

Attachment: WIP Send display list info to the layerscope viewer

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8652165 [details] [diff] [review]
WIP Send display list info to the layerscope viewer

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

Leave the graphics part to morris :P

::: dom/canvas/ImageBitmap.h
@@ +144,5 @@
>  
>    nsCOMPtr<nsIGlobalObject> mParent;
>  
> +//CJKU : heck!
> +public:

Bug#1108950 also needs to access the mData and since this bug is going to land sooner, I guess, please do me a favor to add the getter here.

already_AddRefed<layers::Image> GetImage() const
{
  nsRefPtr<layers::Image> image = mData;
  return image.forget();
}

::: dom/canvas/WebGLTextureUpload.cpp
@@ +717,5 @@
> +WebGLTexture::TexImage2D(TexImageTarget texImageTarget, GLint level, GLenum internalFormat,
> +                        GLenum unpackFormat, GLenum unpackType, dom::ImageBitmap& bitmap,
> +                        ErrorResult* const out_rv)
> +{
> +    layers::Image* srcImage = bitmap.mData;

So, use bitmap.GetImage() here.

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8652165 [details] [diff] [review]
WIP Send display list info to the layerscope viewer

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

LGTM

::: dom/canvas/WebGLTextureUpload.cpp
@@ +720,5 @@
> +{
> +    layers::Image* srcImage = bitmap.mData;
> +
> +    if (TexImageFromImage(texImageTarget, level, internalFormat,
> +                          unpackFormat, unpackType, srcImage))

need brace {
See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
"Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning."

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8652165 [details] [diff] [review]
WIP Send display list info to the layerscope viewer

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

Looks good. Only some nit.

::: dom/canvas/WebGLContext.h
@@ +40,4 @@
>  #include "nsIDOMWebGLRenderingContext.h"
>  #include "nsIObserver.h"
>  
> +#include "ImageBitmap.h"

This is unnecessary. Please use forward declaration.

::: dom/canvas/WebGLTexture.h
@@ +17,4 @@
>  #include "WebGLFramebufferAttachable.h"
>  #include "WebGLObjectModel.h"
>  #include "WebGLStrongTypes.h"
> +#include "ImageBitmap.h"

ditto

::: dom/canvas/WebGLTextureUpload.cpp
@@ +737,5 @@
> +    gfx::IntSize size = dataSurface->GetSize();
> +    const uint32_t byteLength = map.GetStride() * size.height;
> +    return TexImage2D_base(texImageTarget, level, internalFormat, size.width,
> +                           size.height, map.GetStride(), 0,
> +                           unpackFormat, unpackType, map.GetData(), byteLength, js::Scalar::MaxTypedArrayViewType,

This line is too long. Can you move js::Scalar::MaxTypedArrayViewType to next line?

@@ +982,5 @@
> +                             unpackFormat, unpackType, srcImage);
> +}
> +
> +bool
> +WebGLTexture::TexImageFromImage(TexImageTarget texImageTarget, GLint level, GLenum internalFormat,

ditto.

Comment 13

[u459114]

Although this bug is relative to foxeye, the implementation itself is located in canvas::webgl. Correct component this one belong.

Comment 14

[u459114]

Attachment: WIP - Accept ImageBitmap as source buffer of TexImage2D

Comment 15

[Daosheng Mu[:daoshengmu]]

In WebGL worker case, if we would like to support texture mapping, we have to find a way to load images for WebGL context. Currently, the only way we can do is loading images at the main thread and sending them to worker thread. But, it would bring some pain. For example, we have to convert image data into bytearray even some build-in image formats, jpeg, bmp, gif, etc. And then, sending them to the worker thread.

If we could support this feature for developers, we can just load images as ImageBitmap and send them to worker thread without doing any conversion. That would be definitely a big help.

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Jeff, How do you think about comment 15?

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to Daosheng Mu[:daoshengmu] from comment #15)
> In WebGL worker case, if we would like to support texture mapping, we have
> to find a way to load images for WebGL context. Currently, the only way we
> can do is loading images at the main thread and sending them to worker
> thread. But, it would bring some pain. For example, we have to convert image
> data into bytearray even some build-in image formats, jpeg, bmp, gif, etc.
> And then, sending them to the worker thread.
Who's "we" here? Ideally we should give users a way to pull an ImageData out of an ImageBitmap, but I think that's out-of-scope for this bug.

> 
> If we could support this feature for developers, we can just load images as
> ImageBitmap and send them to worker thread without doing any conversion.
> That would be definitely a big help.

It looks like createImageBitmap is (planned to be) implemented both for Window and WorkerGlobalScope.
createImageBitmap can take a Blob, so a dev should be able to XHR an image into a blob, createImageBitmap from it, and have an ImageBitmap on the Worker with no main-thread interactions.

Comment 18

[u459114]

> > If we could support this feature for developers, we can just load images as
> > ImageBitmap and send them to worker thread without doing any conversion.
> > That would be definitely a big help.
> 
> It looks like createImageBitmap is (planned to be) implemented both for
> Window and WorkerGlobalScope.
> createImageBitmap can take a Blob, so a dev should be able to XHR an image
> into a blob, createImageBitmap from it, and have an ImageBitmap on the
> Worker with no main-thread interactions.
Hi Jeff,
Like you said, yes, we can do it by the way you mentioned(you may see my first comment in this bug as well), the only concern is performance. It takes one texture read back and one texture upload under beneath.

If we have 
WebGLContext::TexImage2D(..., dom::ImageBitmap& pixels);

Inside gecko, we can use BlitImageToTexture for texture copy, which has a performance gain on platforms supporting direct texture.

Do you think that have this new API is reasonable?

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to C.J. Ku[:cjku] from comment #18)
> > > If we could support this feature for developers, we can just load images as
> > > ImageBitmap and send them to worker thread without doing any conversion.
> > > That would be definitely a big help.
> > 
> > It looks like createImageBitmap is (planned to be) implemented both for
> > Window and WorkerGlobalScope.
> > createImageBitmap can take a Blob, so a dev should be able to XHR an image
> > into a blob, createImageBitmap from it, and have an ImageBitmap on the
> > Worker with no main-thread interactions.
> Hi Jeff,
> Like you said, yes, we can do it by the way you mentioned(you may see my
> first comment in this bug as well), the only concern is performance. It
> takes one texture read back and one texture upload under beneath.
> 
> If we have 
> WebGLContext::TexImage2D(..., dom::ImageBitmap& pixels);
> 
> Inside gecko, we can use BlitImageToTexture for texture copy, which has a
> performance gain on platforms supporting direct texture.
> 
> Do you think that have this new API is reasonable?

Yes, we're already planning on adding ImageBitmap in the WebGL spec.

How does/would it work without using TexImage2D? Is there a way to get an ImageData or raw ArrayBuffer out of an ImageBitmap?

Comment 20

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

> How does/would it work without using TexImage2D? Is there a way to get an
> ImageData or raw ArrayBuffer out of an ImageBitmap?
Bug 1141979 is working on that. :)

Comment 21

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

> How does/would it work without using TexImage2D? Is there a way to get an
> ImageData or raw ArrayBuffer out of an ImageBitmap?
Bug 1141979 is working on that. :)

Comment 22

[Kirill Dmitrenko]

Hi, guys!

What's status of this bug? Using ImageBitmap with its asynchronous decoding is quite desirable for any application loading textures at runtime to avoid a lot of blocking for substantial amount of time texImage2D calls.