[webkit-reviews] review denied: [Bug 192977] [GStreamer] Add sharedBuffer utility to GstMappedBuffer, and a testsuite : [Attachment 358691] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 192977: [GStreamer] Add sharedBuffer utility to GstMappedBuffer, and a
testsuite
https://bugs.webkit.org/show_bug.cgi?id=192977

Attachment 358691: Patch

https://bugs.webkit.org/attachment.cgi?id=358691&action=review




--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 358691
  --> https://bugs.webkit.org/attachment.cgi?id=358691
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:58
> +Ref<SharedBuffer> SharedBuffer::create(RefPtr<GstMappedBuffer> buffer)

Since buffer can't be nullptr, I think this should receive a const ref instead.
const GstMappedBuffer&

> Source/WebCore/platform/SharedBuffer.cpp:63
> +SharedBuffer::SharedBuffer(RefPtr<GstMappedBuffer>& mappedBuffer)

This would get the const reference too

> Source/WebCore/platform/SharedBuffer.cpp:66
> +    m_segments.append({0, DataSegment::create(WTFMove(mappedBuffer))});

And here we would create the RefPtr to pass it to DataSegment::create

> Source/WebCore/platform/SharedBuffer.cpp:227
> +#if USE(GLIB)

GSTREAMER

> Source/WebCore/platform/SharedBuffer.h:53
> +#if USE(GSTREAMER)
> +#include "GStreamerCommon.h"
> +#endif

Can't we forward declare GstMappedBuffer here?

> Source/WebCore/platform/SharedBuffer.h:156
> +#if USE(GLIB)

GSTREAMER

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:369
> +RefPtr<SharedBuffer> GstMappedBuffer::sharedBuffer()

This is always creating a new SharedBuffer (or nullptr), so I would call this
tryCreateSharedBuffer()

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:83
>      WTF_MAKE_NONCOPYABLE(GstMappedBuffer);

ThreadSafeRefCounted is already non-copyable so I think we can remove this now

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:101
> +    explicit operator bool() const { return m_isValid; }

Now that we have a create method, I wonder whether we still need the m_isValid.
It's set on the constructor and only changes in the destructor. So, create
could call gst_buffer_map() and return nullptr if it fails. Then, the
constructor would receive the GstMapInfo too.