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

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris 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 357935: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 357935
  --> https://bugs.webkit.org/attachment.cgi?id=357935
Patch

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

In a more general note, when I told you it was a good idea to have
GstMappedBuffer::sharedBuffer, I thought you could just cache the
RefPtr<SharedBuffer> inside the GstMappedBuffer. I don't think we need to
complicate the design to create a general cache based on buffers. Another thing
is that I guess we should only create shared buffers when the memory is
readable and writable only.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:361
> +static Lock cachedSharedBuffersLock;
> +static HashMap<GstBuffer*, RefPtr<SharedBuffer>>& cachedSharedBuffers()

Can we have this as staric class members?

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GStreamerTest.cpp:48
> +    // Might be tempting to call gst_deint() here, don't, despite the
naming,
> +    // gst_init() doesn't reverse that call.

It might be my English and my understanding of the subtleties of the word
"reverse" but I don't understand very well this comment.

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:41
> +    GstBuffer* buf = gst_buffer_new_allocate(NULL, 64, NULL);

Please use nullptr and even when this is just a test, we shouldn't leak the
buffer, a GRefPtr should do I guess.

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:65
> +    GstBuffer* buf = gst_buffer_new_wrapped(memory, 16);

Ditto

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:87
> +    GstBuffer* buf = gst_buffer_new_allocate(NULL, 64, NULL);

Ditto and ditto

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:95
> +TEST_F(GStreamerTest, mappedBufferDoesNotAddExtraRefs)

Why do we need this test?

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:97
> +    GstBuffer* buf = gst_buffer_new();

Ditto