918984 - Direct video stream texturing support on WebGL

Status: RESOLVED INCOMPLETE

(Core :: Graphics: CanvasWebGL, defect)

Description

[Sotaro Ikeda [:sotaro]]

On b2g v1.4?(1.3's next), we want to support realtime camera effect on camera app by using WebGL. To do it, video streams direct texturing becomes necessary.

Followings are related links receive from bjacob. Thanks.

http://www.khronos.org/registry/webgl/extensions/proposals/WEBGL_dynamic_texture/

https://www.khronos.org/webgl/public-mailing-list/archives/1306/msg00005.html

Comment 1

[Wayne Chang [:wchang]]

retiring madai flag, setting a suitable target flag.

Comment 2

[Jason Smith [:jsmith]]

Not a blocking issue, as we've shipped with this in previous releases.

Comment 3

[Chiajung Hung [:chiajung]]

To be clarify, I think we should break this into several bugs:

For EGL-backed WebGL only:
(1) Support WebGL_dynamic_texture if EGL_stream and EGL_stream_consumer_gltexture are supported
(2) Support WebGL_dynamic_texture if EGL_image_external and KHR_image_base are supported (most common case)

Both of them need some change in ANGLE's shader validator. The difference of (1) and (2) is that (2) need a fully implemented WDTStream, and (1) may use EGL_stream's functions directly. Thus we will need 2 WDTStream implementation based on the extension combination. I would like to implement (2) in this bug since I have no device that supports the combination of (1).

For globally support, I think there are some options:
For Windows 
(1) Implement EGL_stream and EGL_stream_consumer_gltexture into ANGLE
(2) Use other equivalent functions provided by wgl if exist
For Linux/Mac
(1) Make ANGLE emit color conversion code into the GLSL shader when samplerExternalOES is requested.
(2) Use other equivalent functions provided by xgl/cgl if exist

But basically, the TexImage2D(<video>) case should be fast enough on Desktop since they generally have powerful CPU/GPU.

Comment 4

[Chiajung Hung [:chiajung]]

Attachment: Draft V1

Not work yet, just put here so everyone can see the basic construction of my plan.

I am now implement the video poller stream producer.
Basically, I think it is not good: a better solution would be make HTMLCanvasElement/HTMLVideoElement/HTMLImageElement a WebGLStreamProducer (WebGLStreamActor in my draft patch) directly.

I found that some function in the spec is useless, and some is impossible to be implemented. For example: minFrameDuration is impossible to be implemented, since the Video can be live-stream(WebRTC/GetUserMedia/RTSP/Canvas), it is impossible to calculate such information. And in fact, it is very hard for app authors to estimate how simple their shader should be from this value.

Comment 5

[Chiajung Hung [:chiajung]]

Attachment: Draft V2

Build pass, and I think it not working yet.

@Jeff, Dan, Benoit
As discussed on IRC, here is my basic constructions of this extension. Currently, it needs only egl_image_external, and I plan to add another path for device which supports egl_stream_producer_* + egl_stream + egl_stream_consumer_gltexture, if I can find one such device.

Next part would be shader validator related stuff or showcase app stuff.

Comment 6

[u480271]

Comment on attachment 8437399 [details] [diff] [review]
Draft V2

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

I have to spend some more time looking over the stream interface and logic, but something doesn't feel right to me with the Consumer, Producer and Stream. My intuition tells me there are too many classes. It feels like there should be one class for the Stream object, on which frames/images are "pushed" (produced) and "popped" (consumed). Maybe the details of EGL texture external don't allow that?

::: content/canvas/src/WebGLStreamBase.h
@@ +2,5 @@
> + * WebGLStreamBase.h
> + *
> + *  Created on: May 13, 2014
> + *      Author: chiajung
> + */

File license boilerplater: http://www.mozilla.org/MPL/headers/

@@ +14,5 @@
> +
> +namespace mozilla {
> +
> +namespace dom {
> +class HTMLCanvasElementOrHTMLImageElementOrHTMLVideoElement;

That is truly horrid.

@@ +54,5 @@
> +    nsRefPtr<FrameInfo> mFrameInfo;
> +    void* GetNativeBuffer();
> +};
> +
> +class WebGLStreamActor {

Not a fan of the Actor suffix. But I can't think of a better name. Maybe :jgilbert or :bjacob can think of a different name?

::: content/canvas/src/WebGLTexture.h
@@ +144,1 @@
>                     (target >= LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X &&

How does that compile?

::: dom/webidl/WebGLRenderingContext.webidl
@@ +50,5 @@
> +    readonly attribute WDTStreamFrameInfo producerFrame;
> +
> +    readonly attribute WDTNanoTime minFrameDuration;
> +
> +    readonly attribute GLenum state;  

Trailing whitespace

@@ +55,5 @@
> +
> +    attribute WDTNanoTime acquireTimeout;
> +    attribute WDTNanoTime consumerLatency;
> +
> +    void connectSource((HTMLCanvasElement or HTMLImageElement or HTMLVideoElement) source);

typedef (HTMLImageElement or
         HTMLCanvasElement or
         HTMLVideoElement) StreamImageSource;
void connectSource(StreamImageSource source);
StreamImageSource? getSource();

@@ +864,5 @@
> +    const GLenum TEXTURE_BINDING_EXTERNAL_OES = 0x8D67;
> +    const GLenum REQUIRED_TEXTURE_IMAGE_UNITS_OES = 0x8D68;
> +
> +    WDTStream? createStream();
> + 

Trailing whitespace

Comment 7

[Chiajung Hung [:chiajung]]

Comment on attachment 8437399 [details] [diff] [review]
Draft V2

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

Thanks for the info.

In fact, I feel that I add way too much classes, too. In fact, I had merged some of them...like WebGLStreamProducerBase and WebGLStreamConsumerBase becomes WebGLStreamActor now.
Maybe I can change WebGLTexture directly, and forget the possibility of new consumer.(I think MediaRecoder/WebRTC is another possible consumer, if we want record/share the Canvas output) Also, I can try to get the newest frame from those element directly when AcquireImage calls rather than make them send the image out. But this way it is 98% equal to TexImage2D API.(RBswap/multiply alpha on main thread is still needed)

The most weird class here must be those StreamProducers, since Canvas/Image/Video should be StreamProducer themselves.
However, they produce different type of output object: layers::Image, gfx::Image, SharedSurface. (I hope they all produce TextureClient, and make life easy. NO, nsLayoutUtils::SurfaceFromElement is not an option :p)

Producer side can be very simple, if we make Canvas/Image/Video elements producer themselves. Consumer side can be simple if we forget about the Consumer role just use WebGLTexture.

::: content/canvas/src/WebGLTexture.h
@@ +144,1 @@
>                     (target >= LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X &&

Since MOZ_ASSERT does not compile on release build :p

::: dom/webidl/WebGLRenderingContext.webidl
@@ +50,5 @@
> +    readonly attribute WDTStreamFrameInfo producerFrame;
> +
> +    readonly attribute WDTNanoTime minFrameDuration;
> +
> +    readonly attribute GLenum state;  

For those formatting problem, I will let eclipse clean them up for me before go to review phase.

@@ +55,5 @@
> +
> +    attribute WDTNanoTime acquireTimeout;
> +    attribute WDTNanoTime consumerLatency;
> +
> +    void connectSource((HTMLCanvasElement or HTMLImageElement or HTMLVideoElement) source);

I tried this before and it not work...In fact spec typedef the union StreamSource like what I did in the C++ equivalent

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Cancelling my needinfo, because I'm not anymore working on WebGL, and Dan and Jeff are already needinfo'd.

Comment 9

[u480271]

I'd like to see some feedback from Jeff. Maybe this requires a Vidyo call to discuss?

Comment 10

[Chiajung Hung [:chiajung]]

Attachment: Working Draft

Some dirty trick inside. Some problem remains. 
And one major question:
While normal video status check returns HAVE_NOTHING if mozSrcObject used, what behaviour is expected?

Comment 11

[Chiajung Hung [:chiajung]]

Attachment: WIP showcase

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8441169 [details] [diff] [review]
Working Draft

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

There are a lot of classes in here. It seems like we can get away with fully half the number of classes.
It feels like there's too much abstraction here that runs afoul of YAGNI (You Ain't Gonna Need It). Unless there's good reason to believe these abstractions make things easier to read, or arguably will be useful in the immediate future, we should simplify them out.

In particular, the following classes should probably be removed/merged into other classes:
* One of [WebGLStreamFrameInfo, FrameInfo]
* WebGLStreamActor
* WebGLStreamActor::WebGLStreamProducerListener
* WebGLStreamBase

Maybe a bunch more comments would make this easier to understand, as well?

::: content/canvas/src/WebGLContext.h
@@ +229,5 @@
>  
>      WebGLTexture* activeBoundTextureForTarget(GLenum target) const {
> +        return target == LOCAL_GL_TEXTURE_2D ? mBound2DTextures[mActiveTexture] :
> +               (target == LOCAL_GL_TEXTURE_EXTERNAL ? mBoundExternalTextures[mActiveTexture] :
> +                mBoundCubeMapTextures[mActiveTexture]);

Switch this to use a switch.
Also assert that the extension is activated if target is TEXTURE_EXTERNAL.

::: content/canvas/src/WebGLContextDraw.cpp
@@ +122,4 @@
>      if (IsContextLost())
>          return;
>  
> +    /*if (!ValidateDrawModeEnum(mode, "drawArrays: mode"))

Why is this commented?

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +159,5 @@
>              return WebGLExtensionDrawBuffers::IsSupported(this);
>          case WebGLExtensionID::EXT_frag_depth:
>              return WebGLExtensionFragDepth::IsSupported(this);
> +        case WebGLExtensionID::WEBGL_dynamic_texture:
> +            return WebGLExtensionDynamicTexture::IsSupported(this);

This should at least go behind the 'draft' flag. Technically, it's not even a draft, but let's treat it like one even in prototyping.

::: content/canvas/src/WebGLContextGL.cpp
@@ +223,5 @@
>  
>      if (target == LOCAL_GL_TEXTURE_2D) {
>          currentTexPtr = &mBound2DTextures[mActiveTexture];
> +    } else if (target == LOCAL_GL_TEXTURE_EXTERNAL) {
> +        currentTexPtr = &mBoundExternalTextures[mActiveTexture];

Requires activation of the extension.

@@ +711,5 @@
>      GLuint activeTexture = mActiveTexture;
>      for (int32_t i = 0; i < mGLMaxTextureUnits; i++) {
>          if ((tex->Target() == LOCAL_GL_TEXTURE_2D && mBound2DTextures[i] == tex) ||
> +            (tex->Target() == LOCAL_GL_TEXTURE_CUBE_MAP && mBoundCubeMapTextures[i] == tex) ||
> +            (tex->Target() == LOCAL_GL_TEXTURE_EXTERNAL && mBoundExternalTextures[i] == tex))

Requires activation of the extension.

::: content/canvas/src/WebGLContextState.cpp
@@ +442,5 @@
>              return WebGLObjectAsJSValue(cx, mBound2DTextures[mActiveTexture].get(), rv);
>          }
>  
> +        case LOCAL_GL_TEXTURE_EXTERNAL: {
> +            return WebGLObjectAsJSValue(cx, mBoundExternalTextures[mActiveTexture].get(), rv);

Requires activation of the extension.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +397,4 @@
>      switch (target) {
>          case LOCAL_GL_TEXTURE_2D:
>          case LOCAL_GL_TEXTURE_CUBE_MAP:
> +        case LOCAL_GL_TEXTURE_EXTERNAL:

Requires activation of the extension.

::: content/canvas/src/WebGLExtensionDynamicTexture.cpp
@@ +5,5 @@
> +#include "mozilla/dom/WebGLRenderingContextBinding.h"
> +
> +namespace mozilla {
> +
> +class AutoLog {

I'm assuming this is temporary.

::: content/canvas/src/WebGLStream.cpp
@@ +2,5 @@
> + * WebGLStream.cpp
> + *
> + *  Created on: May 14, 2014
> + *      Author: chiajung
> + */

Use the MPL.

@@ +5,5 @@
> + *      Author: chiajung
> + */
> +
> +#include "mozilla/dom/WebGLRenderingContextBinding.h"
> +#include "WebGLStream.h"

The same-named header include in the source file should go first.

@@ +83,5 @@
> +        this->OnDisconnect();
> +    }
> +    nsRefPtr<WebGLStreamBase> oldStream = mStream;
> +    mStream = aStream;
> +    if (mStream != aStream && mStream) {

Surely `mStream != aStream` is always false at this point?

@@ +127,5 @@
> +    if (source->GetRole() == WebGLStreamActor::ST_PRODUCER && mProducer == nullptr) {
> +        mProducer = source;
> +        mProducer->SetStream(this);
> +    }
> +    if (source->GetRole() == WebGLStreamActor::ST_CONSUMER && mConsumer == nullptr) {

In order to create a WDTStream, we already have an immutable consumer.

@@ +356,5 @@
> +    mPollerThread->Dispatch(NS_NewRunnableMethod(this, &VideoStreamProducer::PollOneFrame), NS_DISPATCH_NORMAL);
> +}
> +
> +// Concreate implementations
> +WebGLStream::WebGLStream(WebGLTexture* consumer)

Please include separator comments between the implementations of functions from different classes. It's really hard to quickly find where one set of implementations ends and the next begins, otherwise.


////////////////////////////////////////////////////////////////////////
// WebGLStream

@@ +363,5 @@
> +    , mConsumerLatency(0)
> +{
> +  AutoLog(__PRETTY_FUNCTION__);
> +  SetIsDOMBinding();
> +      mTextureConsumer = new TextureStreamConsumer(consumer);

It's quite distracting to have all these indentation errors. I know this isn't meant to be a final draft, but please clean this up before asking for feedback.

@@ +466,5 @@
> +WDTNanoTime WebGLStream::MinFrameDuration()
> +{
> +  AutoLog(__PRETTY_FUNCTION__);
> +    // TODO: There is no way to determine correct min frame duration
> +    //       (we will have to decode the whole video stream than determine it)

The spec seems to say that this changes, but starts at +infinity. I think that this is just the min frame duration *so far*.

@@ +510,5 @@
> +    return dom::WDTStreamBinding::Wrap(cx, this);
> +}
> +
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(WebGLStream)

I believe this is supposed to include a list of refPtrs held by the class.

@@ +514,5 @@
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(WebGLStream)
> +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(WebGLStream, AddRef)
> +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(WebGLStream, Release)
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(WebGLStreamFrameInfo)

I'm pretty sure this needs to include a reference to mStream.

::: content/canvas/src/WebGLStream.h
@@ +2,5 @@
> + * WebGLStream.h
> + *
> + *  Created on: May 14, 2014
> + *      Author: chiajung
> + */

MPL

@@ +81,5 @@
> +    EGLImage mImage;
> +    nsRefPtr<WebGLVideoFrame> mCurFrame;
> +};
> +
> +class WebGLStream

Since the purpose of this is to implement WDTStream, we should call it WDTStream, or WebGLDynTexStream, maybe.

@@ +94,5 @@
> +    virtual ~WebGLStream();
> +
> +    void Delete() { Disconnect(); }
> +
> +    bool ConnectSource(const StreamSource& source);

Why is this bool?

@@ +112,5 @@
> +    WDTNanoTime ConsumerLatency();
> +    void SetConsumerLatency(WDTNanoTime latency);
> +
> +    // JS bindings
> +    WebGLContext *GetParentObject() const;

Star-to-left.

@@ +120,5 @@
> +    NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(WebGLStream)
> +    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(WebGLStream)
> +
> +protected:
> +    const StreamSource* mSource;

Why is this a bare pointer? Leave a comment about why the lifetimes make this ok.

::: content/canvas/src/WebGLStreamBase.h
@@ +2,5 @@
> + * WebGLStreamBase.h
> + *
> + *  Created on: May 13, 2014
> + *      Author: chiajung
> + */

MPL

@@ +18,5 @@
> +class HTMLCanvasElementOrHTMLImageElementOrHTMLVideoElement;
> +}
> +
> +typedef dom::HTMLCanvasElementOrHTMLImageElementOrHTMLVideoElement
> +  StreamSource;

4-space or 2*4-space for line continuations.

@@ +20,5 @@
> +
> +typedef dom::HTMLCanvasElementOrHTMLImageElementOrHTMLVideoElement
> +  StreamSource;
> +
> +typedef double WDTNanoTime;

We should be pulling this from a central header file.

@@ +43,5 @@
> +        return mPresentTime;
> +    }
> +protected:
> +    double mFrameTime;
> +    WDTNanoTime mPresentTime;

It looks like these should be const.

@@ +46,5 @@
> +    double mFrameTime;
> +    WDTNanoTime mPresentTime;
> +};
> +
> +class WebGLVideoFrame {

It seems like this isn't limited to videos. Maybe WebGLDynTexFrame?

@@ +60,5 @@
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebGLStreamActor)
> +
> +    enum StreamRole {
> +        ST_PRODUCER,
> +        ST_CONSUMER,

These two rolls should almost certainly have different classes.

@@ +64,5 @@
> +        ST_CONSUMER,
> +    };
> +
> +    class WebGLStreamProducerListener {
> +        NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebGLStreamActor))

Oops and/or what?

@@ +98,5 @@
> +    nsRefPtr<WebGLStream> mStream;
> +};*/
> +
> +//TODO: Make the function name more EGLStream like and non-WDTStream like
> +class WebGLStreamBase

Why is this separate from WebGLStream?

@@ +99,5 @@
> +};*/
> +
> +//TODO: Make the function name more EGLStream like and non-WDTStream like
> +class WebGLStreamBase
> +    : public WebGLStreamActor::WebGLStreamProducerListener

Why do we use this mixin?
Why don't we just add thsi function to this class directly?

::: content/canvas/src/WebGLTexture.h
@@ +140,4 @@
>      static size_t FaceForTarget(GLenum target) {
>          // Call this out explicitly:
>          MOZ_ASSERT(target != LOCAL_GL_TEXTURE_CUBE_MAP);
> +        MOZ_ASSERT(target == LOCAL_GL_TEXTURE_2D || target == LOCAL_GL_TEXTURE_EXTERNAL

Assert that the extension is active if target is TEXTURE_EXTERNAL.

::: dom/webidl/WebGLRenderingContext.webidl
@@ +50,5 @@
> +    readonly attribute WDTStreamFrameInfo producerFrame;
> +
> +    readonly attribute WDTNanoTime minFrameDuration;
> +
> +    readonly attribute GLenum state;  

Clear out this whitespace, here and elsewhere.

@@ +55,5 @@
> +
> +    attribute WDTNanoTime acquireTimeout;
> +    attribute WDTNanoTime consumerLatency;
> +
> +    void connectSource((HTMLCanvasElement or HTMLImageElement or HTMLVideoElement) source);

Did the typedef not work?

Comment 13

[Chiajung Hung [:chiajung]]

Thank for the feedback, I will shape it better before next update.

The mass of class is because of spec out-of-sync :p
Basically, I want to support this extension with at least 2 configuration:
1. HW supports: egl_external_image
2. HW supports: egl_stream + egl_stream_consumer (I believe this one is the true EGL correspondence of this spec)
So those base classes are abstraction for support configuration (2), and current implementation is based on configuration (1).

I think this can be re-factored to support new configurations later, I will try to make it simpler in next version.

Thanks for clarify the frameDuration things. For MinFrameDuration, we will have to record/get some statistics from MediaElement, since the polling model here I used can not provide accurate estimation.

Yes, typedef of union in webidl seems not work. 
By the way, I found 2D canvas context has almost same union with class sequence tweaked. Should we unify them to make code size smaller? (The union seems create 2 classes in my patch since the union can be a return value in this extension and 1 class in 2D canvas context)

Another problem about webidl is that the webidl typedefed WDTTime to double, but I can not use WDTTime in my c++ counterpart.

BTW, there is a problem in the JS part: the spec uses #extension OES_EGL_image_external in shader, but ANGLE recognizes only #extension GL_OES_EGL_image_external, which may need some fix in spec or in ANGLE.