| Summary: | [GStreamer] Client-side video rendering doesn't fallback to internal compositing | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||
| Component: | Platform | Assignee: | Philippe Normand <pnormand> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, sergio, vjaquez, webkit-bug-importer, zan | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Philippe Normand
2020-02-04 09:52:43 PST
Created attachment 389674 [details]
Patch
Comment on attachment 389674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389674&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3172 > + auto internalCompositingOperation = [this](TextureMapperPlatformLayerProxy& proxy, std::unique_ptr<GstVideoFrameHolder> frameHolder) { You're moving the frameholder when calling this, so I guess you should make this && as well. Anyway, why do you create a lambda instead of a method considering that we are only capturing this? Comment on attachment 389674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389674&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:218 > + static std::once_flag onceFlag; > + std::call_once(onceFlag, [] { Nit: I'd recommend using `s_onceFlag` as the name, the `s_` prefix more explicitly describing the storage nature. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:372 > + return m_dmabufFD; This should test for m_dmabufFD being >= 0, i.e. a valid fd. Related to this, m_dmabufFD should be initialized to -1 and in handoffVideoDmaBuf() reset to -1 after the hand-off and closure. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3203 > + std::unique_ptr<GstVideoFrameHolder> frameHolder = makeUnique<GstVideoFrameHolder>(m_sample.get(), m_videoDecoderPlatform, m_textureMapperFlags, !m_isUsingFallbackVideoSink); > + if (frameHolder->hasDMABuf()) { > + std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer = makeUnique<TextureMapperPlatformLayerBuffer>(0, m_size, TextureMapperGL::ShouldNotBlend, GL_DONT_CARE); Could use `auto object = makeUnique<>()` in both cases. Comment on attachment 389674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389674&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3172 >> + auto internalCompositingOperation = [this](TextureMapperPlatformLayerProxy& proxy, std::unique_ptr<GstVideoFrameHolder> frameHolder) { > > You're moving the frameholder when calling this, so I guess you should make this && as well. > > Anyway, why do you create a lambda instead of a method considering that we are only capturing this? I don't really see the point of adding a method for this. As the logic remains self-contained in pushTextureToCompositor() anyway. This new closure is an implementation detail of it. Committed r255790: <https://trac.webkit.org/changeset/255790> |