Bug 117118 – [CoordGraphics][GStreamer] Composited Video support

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit EFL

[Kalyan]

After r150890 Hardware accelerated video composition is available when coordinated graphics is not used. This bug is to enable the same support when coordinated graphics is used.

[Kalyan]

Created attachment 203503 [details]
patchv1

[Kalyan]

The patch works but one issue which need to be addressed:

Un-necessary flipping. Once before texSubImage upload (converting buffer to opengl coordinates) and next time during rendering of texture to surface. We can avoid this if we are able to somehow inform GraphicsSurface that flipping is not needed.

[EFL EWS Bot]

Comment on attachment 203503 [details]
patchv1

Attachment 203503 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/723560

[EFL EWS Bot]

Comment on attachment 203503 [details]
patchv1

Attachment 203503 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/656652

[Early Warning System Bot]

Comment on attachment 203503 [details]
patchv1

Attachment 203503 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/723561

[Early Warning System Bot]

Comment on attachment 203503 [details]
patchv1

Attachment 203503 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/656654

[Kalyan]

Created attachment 203532 [details]
patchv2

including missed changes.

[Kalyan]

Created attachment 203544 [details]
patchv3

[Noam Rosenthal]

Comment on attachment 203544 [details]
patchv3

View in context: https://bugs.webkit.org/attachment.cgi?id=203544&action=review

Looks great, though I would ask to find a way to not introduce a new duplication of RGBA->BGRA swizzle.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:388
> +    for (int i = 0; i < height / 2; ++i) {
> +        for (int j = 0; j < width; ++j) {
> +            unsigned tmp = buf[i * width + j];
> +            buf[i * width + j] = buf[(height - i - 1) * width + j];
> +            buf[(height - i - 1) * width + j] = tmp;
> +        }
> +    }

Is this the 10th RGBA->BGRA swizzle code path we have in WebCore? Maybe we can do some reusing?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:661
> +    return m_context3D ? true : false;

The phrase ? true : false doesn't add anything.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:426
> +    m_canvasPlatformLayer->setClient(this);

Maybe this member should be renamed?

[Kalyan]

Comment on attachment 203544 [details]
patchv3

View in context: https://bugs.webkit.org/attachment.cgi?id=203544&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:662
> +#else

This would ensure a safe fallback to non-accelerated path if context3d creation fails(i.e Graphics_Surface usage is disabled or context creation failed for some reason).

[Philippe Normand]

Comment on attachment 203544 [details]
patchv3

View in context: https://bugs.webkit.org/attachment.cgi?id=203544&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:388
>> +    }
> 
> Is this the 10th RGBA->BGRA swizzle code path we have in WebCore? Maybe we can do some reusing?

Perhaps this conversion can be avoided by configuring the video-sink sink pad template caps according to what format the graphics context supports.

[Kalyan]

Created attachment 203589 [details]
patchv4

[Kalyan]

(In reply to comment #9)
> (From update of attachment 203544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203544&action=review
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:426
> > +    m_canvasPlatformLayer->setClient(this);
> 
> Maybe this member should be renamed?

renamed m_canvasPlatformLayer to m_platformLayer

[Kalyan]

(In reply to comment #11)
> (From update of attachment 203544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203544&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:388
> >> +    }
> > 
> > Is this the 10th RGBA->BGRA swizzle code path we have in WebCore? Maybe we can do some reusing?
> 
> Perhaps this conversion can be avoided by configuring the video-sink sink pad template caps according to what format the graphics context supports.

k, I left it out of this patch as we need to handle it for both (when Texture mapper is used and GraphicsContext3D). Would it be k to handle this as a separate issue??

[Philippe Normand]

The pixel format conversion should really be done in the gst pipeline for the coord graphics rendering code path.
For the texturemapper code path I don't think any change in the video-sink is needed for now.

[Kalyan]

(In reply to comment #15)
> The pixel format conversion should really be done in the gst pipeline for the coord graphics rendering code path.
> For the texturemapper code path I don't think any change in the video-sink is needed for now.

I dont see the difference with coordinate graphics and when using Bitmap Texture(thats without coordinate graphics). With Bitmap Texture the conversion would happen in updateContents call(void MediaPlayerPrivateGStreamerBase::updateTexture). It's a general issue rather than being coordinate graphics specific.

[Kalyan]

Comment on attachment 203589 [details]
patchv4

Removing request for review till 117250 is fixed.

[Kalyan]

Created attachment 204116 [details]
patch

temp patch showing only mediastreamer changes

[Kalyan]

Created attachment 206383 [details]
patchv5

[Early Warning System Bot]

Comment on attachment 206383 [details]
patchv5

Attachment 206383 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1043276

[Early Warning System Bot]

Comment on attachment 206383 [details]
patchv5

Attachment 206383 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/925352

[Kalyan]

Created attachment 206387 [details]
patchv6

qt build fix

[Kalyan]

Created attachment 206391 [details]
patchv7

[Build Bot]

Comment on attachment 206391 [details]
patchv7

Attachment 206391 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/931543

New failing tests:
http/tests/security/cross-origin-plugin-private-browsing-toggled.html

[Build Bot]

Created attachment 206464 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Philippe Normand]

Ping? What's the status of this patch?

[Kalyan]

(In reply to comment #26)
> Ping? What's the status of this patch?

Sorry, missed your comment.It will take some time before I can get back to this. I am ok, if you want to take over this work.

[Philippe Normand]

Yoon, should we close this bug?

Perhaps it's somehow still valid for EFL. I don't plan to work on this though.

[Philippe Normand]

Comment on attachment 206391 [details]
patchv7

Pulling out of review-queue, this patch clearly needs an update after 2 years of inactivity.

[Gyuyoung Kim]

(In reply to comment #28)
> Yoon, should we close this bug?
> 
> Perhaps it's somehow still valid for EFL. I don't plan to work on this
> though.

I don't know if there is volunteer to support this patch for EFL port. If there is no contributor for this patch, it would be good to close this bug.

[Gwang Yoon Hwang]

(In reply to comment #30)
> (In reply to comment #28)
> > Yoon, should we close this bug?
> > 
> > Perhaps it's somehow still valid for EFL. I don't plan to work on this
> > though.
> 
> I don't know if there is volunteer to support this patch for EFL port. If
> there is no contributor for this patch, it would be good to close this bug.

I agree to close this bug. It is a little bit diverged already, and we can create/open a bug if it is needed again.