Bug 237328

Created attachment 453504 [details] WIP patch

Created attachment 453709 [details] Patch

Comment on attachment 453709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453709&action=review Maybe I'm being overly cautious, but there's just too much unused and untested code here and I have too many questions to grant a review. I think there are a few options that make sense to me: - Split this into multiple patches and introduce API tests to verify expected behaviour. - Reduce the size of this patch so there's less unused code added in one go and be a bit more liberal with comments/asserts. - Submit a more complete patch where the code is exercised and it's more obvious the reasons for things because there's more surrounding context to judge it. For the record, I like where this patch is going, I just don't think this is ready to land quite yet. The detailed changelog entry is great and appreciated. > Source/WebCore/platform/graphics/gbm/DMABufFormat.h:98 > + uint32_t planeWidth(unsigned planeIndex, uint32_t width) const > + { > + return (width >> planes[planeIndex].horizontalSubsampling); > + } > + > + uint32_t planeHeight(unsigned planeIndex, uint32_t height) const > + { > + return (height >> planes[planeIndex].verticalSubsampling); > + } I don't know why someone might ever do that, but maybe worth asserting that planeIndex < numPlanes here. > Source/WebCore/platform/graphics/gbm/DMABufFormat.h:156 > +inline DMABufFormat DMABufFormat::create<DMABufFormat::FourCC::XRGB8888>() Given that none of these create functions are used in this patch, maybe it's worth introducing them with the patch that uses them? I'm wary of adding this much unused, untested code, even if it's as simple as this. > Source/WebCore/platform/graphics/gbm/DMABufFormat.h:364 > + break; Should we ASSERT_NOT_REACHED() here? > Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:65 > + this->~DMABufReleaseFlag(); > + new (this) DMABufReleaseFlag(WTFMove(o)); This is quite clever, but it seems fragile to future possible constructor/destructor changes. It's also not a form that exists anywhere else in WebCore. I can't think of an alternative that isn't more verbose, however, so maybe my dislike is irrational. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:68 > +static PFNEGLCREATEIMAGEKHRPROC createImageKHR() > +{ > + static PFNEGLCREATEIMAGEKHRPROC s_createImageKHR; > + static std::once_flag s_flag; > + std::call_once(s_flag, > + [&] { > + s_createImageKHR = reinterpret_cast<PFNEGLCREATEIMAGEKHRPROC>(eglGetProcAddress("eglCreateImageKHR")); > + }); > + return s_createImageKHR; > +} > + > +static PFNEGLDESTROYIMAGEKHRPROC destroyImageKHR() > +{ > + static PFNEGLDESTROYIMAGEKHRPROC s_destroyImageKHR; > + static std::once_flag s_flag; > + std::call_once(s_flag, > + [&] { > + s_destroyImageKHR = reinterpret_cast<PFNEGLDESTROYIMAGEKHRPROC>(eglGetProcAddress("eglDestroyImageKHR")); > + }); > + return s_destroyImageKHR; > +} Is there a reason to add these instead of using the ones in GLContextEGL? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:122 > + // Avoid any funky situation where the two layers are the same. Can this legitimately happen? Should we assert something here? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:152 > + if (hasStaleLayers) { > + LayerMap updatedLayers; > + for (auto it = m_layers.begin(); it != m_layers.end(); ++it) { > + if (!isStaleLayer(it->value.get())) > + updatedLayers.add(it->key, it->value.copyRef()); > + } > + m_layers = WTFMove(updatedLayers); > + } Is there any reason not to use HashMap::removeIf here instead of recreating LayerMap? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:159 > + // Avoid any funky situation where the two layers are the same. Same question about legitimacy/asserting. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:175 > + m_object = DMABufObject(0); It doesn't look like this function calls out to anything, is there any reason to explicitly destroy this here? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:190 > + m_imageData = nullptr; If we're explicitly clearing this, may as well put it inside the above block - but I'd suggest just omitting it unless there's a specific reason. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:220 > + // TODO: YUV support, possible when different colorspaces are supported. I'd add an ASSERT_NOT_REACHED to go with this TODO. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:250 > + attributes[attrIndex++] = EGL_WIDTH; > + attributes[attrIndex++] = object.format.planeWidth(i, object.width); > + attributes[attrIndex++] = EGL_HEIGHT; > + attributes[attrIndex++] = object.format.planeHeight(i, object.height); > + attributes[attrIndex++] = EGL_LINUX_DRM_FOURCC_EXT; > + attributes[attrIndex++] = EGLint(object.format.planes[i].fourcc); > + attributes[attrIndex++] = EGL_DMA_BUF_PLANE0_FD_EXT; > + attributes[attrIndex++] = object.fd[i]; > + attributes[attrIndex++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT; > + attributes[attrIndex++] = object.offset[i]; > + attributes[attrIndex++] = EGL_DMA_BUF_PLANE0_PITCH_EXT; > + attributes[attrIndex++] = object.stride[i]; > + attributes[attrIndex++] = EGL_NONE; Using the existing GLContextEGL::createImage would allow you to use the nicer attributes construction that alexg added - see platform/graphics/texmap/TextureMapperPlatformLayerDmabuf.cpp

Comment on attachment 453709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453709&action=review >> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:98 >> + } > > I don't know why someone might ever do that, but maybe worth asserting that planeIndex < numPlanes here. OK. >> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:156 >> +inline DMABufFormat DMABufFormat::create<DMABufFormat::FourCC::XRGB8888>() > > Given that none of these create functions are used in this patch, maybe it's worth introducing them with the patch that uses them? I'm wary of adding this much unused, untested code, even if it's as simple as this. I don't think it makes sense having to revisit plane definitions every time a new format is introduced. That's why they are all provided here, from the beginning. >> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:364 >> + break; > > Should we ASSERT_NOT_REACHED() here? Can't guarantee every FourCC value (e.g. something from GStreamer) will be covered here. So an 'invalid' DMABufFormat is returned. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:68 >> +} > > Is there a reason to add these instead of using the ones in GLContextEGL? Yes, there's no construction of GLContextEGL. And they would be eliminated once libepoxy is adopted by the GTK port. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:122 >> + // Avoid any funky situation where the two layers are the same. > > Can this legitimately happen? Should we assert something here? I don't know, but it can be prevented if it ever does. Debug-time assert can be used to signal it. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:152 >> + } > > Is there any reason not to use HashMap::removeIf here instead of recreating LayerMap? It should be possible to use it. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:175 >> + m_object = DMABufObject(0); > > It doesn't look like this function calls out to anything, is there any reason to explicitly destroy this here? I'll remove this. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:190 >> + m_imageData = nullptr; > > If we're explicitly clearing this, may as well put it inside the above block - but I'd suggest just omitting it unless there's a specific reason. I'll remove this too, and try and push the texture/image destruction into the EGLImageData dtor. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:220 >> + // TODO: YUV support, possible when different colorspaces are supported. > > I'd add an ASSERT_NOT_REACHED to go with this TODO. OK. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:250 >> + attributes[attrIndex++] = EGL_NONE; > > Using the existing GLContextEGL::createImage would allow you to use the nicer attributes construction that alexg added - see platform/graphics/texmap/TextureMapperPlatformLayerDmabuf.cpp Not going to construct it. But I'll switch to using Vector<>.

(In reply to Chris Lord from comment #3) > Comment on attachment 453709 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453709&action=review > > Maybe I'm being overly cautious, but there's just too much unused and > untested code here and I have too many questions to grant a review. I think > there are a few options that make sense to me: > - Split this into multiple patches and introduce API tests to verify > expected behaviour. > - Reduce the size of this patch so there's less unused code added in one go > and be a bit more liberal with comments/asserts. > - Submit a more complete patch where the code is exercised and it's more > obvious the reasons for things because there's more surrounding context to > judge it. > > For the record, I like where this patch is going, I just don't think this is > ready to land quite yet. The detailed changelog entry is great and > appreciated. > I'll upload the updated patch. Review it if you have time, but I'd prefer to roll it into a bigger patch at this point, not smaller ones.

Created attachment 453729 [details] Patch

(In reply to Zan Dobersek from comment #5) > I'll upload the updated patch. Review it if you have time, but I'd prefer to > roll it into a bigger patch at this point, not smaller ones. That sounds good, I'll wait for that.

Created attachment 453839 [details] Patch

Created attachment 454329 [details] Patch

Comment on attachment 454329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454329&action=review Awesome patch! Thanks, let's do it! Just added a couple of small comments. > Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:38 > + enum InitializeTag { Initialize }; > + DMABufReleaseFlag(InitializeTag) InitializeTag is not used, is it a left-over? > Source/WebCore/platform/graphics/gbm/GBMBufferSwapchain.cpp:47 > + // If the description of the requested byffers has changed, update the description to the new one and wreck the existing buffers. Typo byffers. > Source/WebCore/platform/graphics/gbm/GBMBufferSwapchain.cpp:142 > + m_state.releaseFlag = DMABufReleaseFlag(DMABufReleaseFlag::Initialize); Ditto bout the InitializeTag. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:123 > +void TextureMapperPlatformLayerProxyDMABuf::invalidate() > +{ > + Locker locker { m_lock }; > + > + m_pendingLayer = nullptr; > + m_committedLayer = nullptr; > + m_layers = { }; > + > + m_compositor = nullptr; > + m_targetLayer = nullptr; > +} If we don't set m_compositorThread to nullptr we can be asserting if the layer tree is reused when the threaded compositor is destroyed, it can happen when using the history.

Comment on attachment 454329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454329&action=review >> Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:38 >> + DMABufReleaseFlag(InitializeTag) > > InitializeTag is not used, is it a left-over? It's not unused, it's the enumeration type.

Created attachment 454674 [details] Patch for landing

zan@falconsigh.net does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json. Rejecting attachment 454674 [details] from commit queue.

Committed r291270 (248419@main): <https://commits.webkit.org/248419@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454674 [details].

<rdar://problem/90295492>

Rolled out in #237908.

Created attachment 454821 [details] Patch

Comment on attachment 454821 [details] Patch LGTM. Let's try it again. We need to link here the next patch with the gbm guards.

Committed r291343 (248482@main): <https://commits.webkit.org/248482@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454821 [details].