Bug 192977 – [GStreamer] Add sharedBuffer utility to GstMappedBuffer, and a testsuite

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

[GStreamer] Add sharedBuffer utility to GstMappedBuffer, and a testsuite

[Charlie Turner]

Created attachment 357935 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 357935 [details]
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

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #2)
> 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.

That's a better idea, thanks. I attempted to follow carlosgc's idea of adding an overload in the SharedBuffer class, but that requires a number of changes that didn't work well with our requirements managing GStreamer buffers. I went with a ivar rather than a static, since it simplifies the lifetime management, and the caching is not shared among all instances of this class.

> Another
> thing is that I guess we should only create shared buffers when the memory
> is readable and writable only.

I took this to mean we only create them when they are readable and not writable. SharedBuffer provides read-only views of immutable data, so this seems like a good check to add.

> 
> > 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.

I'll rephrase, but after gst_deinit(), the entire process is destroyed from GStreamer's POV, you can't, in the same process, call gst_init() and expect a functional GStreamer.

> 
> > 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.

Done.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:95
> > +TEST_F(GStreamerTest, mappedBufferDoesNotAddExtraRefs)
> 
> Why do we need this test?

I'll add a comment explaining more, but it's motivated by mapping writable buffers. If you increase the refcount, you will invalidate writability, and user-code in GStreamer will break. So I want this class to have a "weak reference" to the GstBuffer rather than messing with its refcount.
>

[Charlie Turner]

Created attachment 358495 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 358495 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:365
> +    m_cachedSharedBuffer = nullptr;

The smart pointer will to its magic, I don't think you need this.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:375
> +    if (!m_isValid || m_info.flags & GST_MAP_WRITE)
> +        return nullptr;

I am quite certain that we want to ASSERT_NOT_REACHED before returning the nullptr

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GStreamerTest.cpp:42
> +        gst_init(NULL, NULL);

nullptr

> Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:95
> +    EXPECT_EQ(sharedBuf.get(), mappedBuf.sharedBuffer().get());
> +    EXPECT_EQ(sharedBuf.get(), mappedBuf.sharedBuffer().get());

What's the purpose of running the same line twice?

[Carlos Garcia Campos]

Comment on attachment 358495 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:378
> +    if (!m_cachedSharedBuffer)
> +        m_cachedSharedBuffer = SharedBuffer::create(data(), size());

I still think we can avoid the data copy here. If I understood this correctly, the buffer is immutable now, so we can create a SharedBuffer wrapping the data. If we don't want to add a new SharedBuffer constructor for a GstMappedBuffer, we can use GBytes, with a free function to unref the GstMappedBuffer (needs to be refcounted). Then we would only cache the GBytes, that would be used to create SharedBuffers, all of them taking a ref of the same GBytes. Or do we really need to always return the same SharedBuffer instance?

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #5)
> Comment on attachment 358495 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358495&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:365
> > +    m_cachedSharedBuffer = nullptr;
> 
> The smart pointer will to its magic, I don't think you need this.

Yep, I wanted to be explicit about it, but I'll remove the line.

> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:375
> > +    if (!m_isValid || m_info.flags & GST_MAP_WRITE)
> > +        return nullptr;
> 
> I am quite certain that we want to ASSERT_NOT_REACHED before returning the
> nullptr

Will fix.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GStreamerTest.cpp:42
> > +        gst_init(NULL, NULL);
> 
> nullptr

Ditto.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/gstreamer/GstMappedBuffer.cpp:95
> > +    EXPECT_EQ(sharedBuf.get(), mappedBuf.sharedBuffer().get());
> > +    EXPECT_EQ(sharedBuf.get(), mappedBuf.sharedBuffer().get());
> 
> What's the purpose of running the same line twice?

It's to ensure the sharedBuffer() method really is returning the same SharedBuffer object. Once is enough, it's just being extra defensive.

[Charlie Turner]

(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 358495 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358495&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:378
> > +    if (!m_cachedSharedBuffer)
> > +        m_cachedSharedBuffer = SharedBuffer::create(data(), size());
> 
> I still think we can avoid the data copy here. If I understood this
> correctly, the buffer is immutable now, so we can create a SharedBuffer
> wrapping the data. If we don't want to add a new SharedBuffer constructor
> for a GstMappedBuffer, we can use GBytes, with a free function to unref the
> GstMappedBuffer (needs to be refcounted).

OK, I will try it this way

> Then we would only cache the
> GBytes, that would be used to create SharedBuffers, all of them taking a ref
> of the same GBytes. Or do we really need to always return the same
> SharedBuffer instance?

No not really, having the same shared buffer instance so far is just used in the API test to check caching is really happening.

[Charlie Turner]

Created attachment 358691 [details]
Patch

Try again with SharedBuffer(GstMappedBuffer) overload. The only problem here is that, to break the cyclic dependency on SharedBuffer and GstMappedBuffer, SharedBuffer is declared as opaque in GStreamerCommon.h, so if you don't include both GStreamerCommon.h *and* SharedBuffer.h in the usage code, then you will get errors

[Carlos Garcia Campos]

Comment on attachment 358691 [details]
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.

[Xabier Rodríguez Calvar]

Comment on attachment 358691 [details]
Patch

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

It's ok that we do this to avoid the copy from the GstMapperBuffer but our current use cases don't imply, AFAIK, creating SharedBuffers all the time. It's just for a couple of places, to report init datas (less that 1k)  maybe twice every couple of minutes. So I wonder if we are overengineering this.

>> Source/WebCore/platform/SharedBuffer.h:53
>> +#endif
> 
> Can't we forward declare GstMappedBuffer here?

If we can't, I think we should move the GstMappedBuffer to another file and we'd fix or at least minimize fuzz, right?

> Source/WebCore/platform/SharedBuffer.h:158
> +            : m_immutableData(WTFMove(data)) { }

If we go this way, we need to ASSERT here somehow that the GstMappedBuffer is valid and readonly cause otherwise you would be able to create a SharedBuffer from a GstMappedBuffer without using the ::sharedBuffer() (or the ::tryCreateSharedBuffer()) method.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:-346
> -        ASSERT(mappedBuffer);

Why did we drop this?

>> 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.

We need to know, at least, if the constructor was able to map the memory. Then there's also the move constructor, etc.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:116
> +        ASSERT(mappedBuffer.get());

This looks uglier now :/ . We might want to have an isValid() method.

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #11)
> Comment on attachment 358691 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358691&action=review
> 
> It's ok that we do this to avoid the copy from the GstMapperBuffer but our
> current use cases don't imply, AFAIK, creating SharedBuffers all the time.
> It's just for a couple of places, to report init datas (less that 1k)  maybe
> twice every couple of minutes. So I wonder if we are overengineering this.

Yes, yes we are :-). One benefit it that it's a better citizen of WebCore like this at least.
 
> >> Source/WebCore/platform/SharedBuffer.h:53
> >> +#endif
> > 
> > Can't we forward declare GstMappedBuffer here?

Nope, the Variant in DataSegment needs to see the class derives from RefCounted for the ->deref() call unfortunately.
> 
> If we can't, I think we should move the GstMappedBuffer to another file and
> we'd fix or at least minimize fuzz, right?
> 
> > Source/WebCore/platform/SharedBuffer.h:158
> > +            : m_immutableData(WTFMove(data)) { }
> 
> If we go this way, we need to ASSERT here somehow that the GstMappedBuffer
> is valid and readonly cause otherwise you would be able to create a
> SharedBuffer from a GstMappedBuffer without using the ::sharedBuffer() (or
> the ::tryCreateSharedBuffer()) method.
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:-346
> > -        ASSERT(mappedBuffer);
> 
> Why did we drop this?

Reinstated.

> 
> >> 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.
> 
> We need to know, at least, if the constructor was able to map the memory.
> Then there's also the move constructor, etc.

Move ctor etc have been removed now that we're derived from ThreadSafeRefCounted. I have followed cgarcia's suggestion in my new patch.

> 
> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:116
> > +        ASSERT(mappedBuffer.get());
> 
> This looks uglier now :/ . We might want to have an isValid() method.

Thought so too, now the operator bool() is gone, the checks become whether the return RefPtr != nullptr, so that looks like ASSERT(mappedBuffer) again. There's no longer an m_isValid member after using cgarcia's review comment.

[Charlie Turner]

(In reply to Carlos Garcia Campos from comment #10)
> Comment on attachment 358691 [details]
> 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&

Can't be const ref because RefPtr only accepts non-const pointers, but I changed it to a ref, so at least it can't be null now.


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

Whoops, fixed.

> 
> > Source/WebCore/platform/SharedBuffer.h:53
> > +#if USE(GSTREAMER)
> > +#include "GStreamerCommon.h"
> > +#endif
> 
> Can't we forward declare GstMappedBuffer here?

See above comment, but no we can't :/

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

Done.
> 
> > 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

Done.

> 
> > 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.

Done, that eliminates the need for m_isValid now, thanks.

[Charlie Turner]

Created attachment 358770 [details]
Patch

There are couple failures in WebRTC because I don't build with that enabled, will fix next, this addresses last review comments

[Charlie Turner]

Created attachment 358774 [details]
Patch

Fix failures in WebRTC

[Carlos Garcia Campos]

Comment on attachment 358774 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.h:137
> +        static Ref<DataSegment> create(RefPtr<GstMappedBuffer> data) { return adoptRef(*new DataSegment(WTFMove(data))); }

&&

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:366
> +        ASSERT_NOT_REACHED();

Why do we assert? is the caller expected to do the check? If that's the case then I think it's better to return Ref<> and use release assert (and remove the 'try' from the name). Then we can remove all the null-checks in the callers.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:89
> +        return adoptRef(new GstMappedBuffer(buffer, info));

WTFMove(info)

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:111
> +    GstMappedBuffer(GstBuffer* buffer, GstMapInfo info)

GstMapInfo&&

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:112
> +        : m_buffer(buffer), m_info(info)

Use two lines here and WTFMove(info)

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:-124
> -    bool m_isValid { false };

o/\o

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:117
> +        ASSERT(mappedBuffer.get());
> +        if (!mappedBuffer.get()) {

You can remove the .get() here now.

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:48
> +        m_payload = mappedInitData->tryCreateSharedBuffer();

m_payload is indeed used without any null-check in append().

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:231
> +    webKitMediaClearKeyDecryptorFindAndSetKey(priv, mappedKeyIdBuffer.releaseNonNull());

Why not move instead? and make webKitMediaClearKeyDecryptorFindAndSetKey receive a RefPtr<>&& ?

[Xabier Rodríguez Calvar]

Comment on attachment 358774 [details]
Patch

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

Other than a couple cosmetic/minor things, LGTM

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:89
> +        return adoptRef(new GstMappedBuffer(buffer, info));

I'd move the info here

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:112
> +    GstMappedBuffer(GstBuffer* buffer, GstMapInfo info)
> +        : m_buffer(buffer), m_info(info)

GstMapInfo&& and move to avoid a double copy

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:117
> +        ASSERT(mappedBuffer.get());
> +        if (!mappedBuffer.get()) {

I guess you don't need the .get() here.

[Charlie Turner]

(In reply to Carlos Garcia Campos from comment #16)
> Comment on attachment 358774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358774&action=review
> 
> > Source/WebCore/platform/SharedBuffer.h:137
> > +        static Ref<DataSegment> create(RefPtr<GstMappedBuffer> data) { return adoptRef(*new DataSegment(WTFMove(data))); }
> 
> &&

Done.

> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:366
> > +        ASSERT_NOT_REACHED();
> 
> Why do we assert? is the caller expected to do the check? If that's the case
> then I think it's better to return Ref<> and use release assert (and remove
> the 'try' from the name). Then we can remove all the null-checks in the
> callers.

Completely agree, much better to assert here and not make callers null check. Done.
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:89
> > +        return adoptRef(new GstMappedBuffer(buffer, info));
> 
> WTFMove(info)

Done.

> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:111
> > +    GstMappedBuffer(GstBuffer* buffer, GstMapInfo info)
> 
> GstMapInfo&&

Done.

> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:112
> > +        : m_buffer(buffer), m_info(info)
> 
> Use two lines here and WTFMove(info)

Done.

> 
> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:117
> > +        ASSERT(mappedBuffer.get());
> > +        if (!mappedBuffer.get()) {
> 
> You can remove the .get() here now.

Yep, done.

> 
> > Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:48
> > +        m_payload = mappedInitData->tryCreateSharedBuffer();
> 
> m_payload is indeed used without any null-check in append().

Indeed, no longer a problem with the RELEASE_ASSERT now :)

> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:231
> > +    webKitMediaClearKeyDecryptorFindAndSetKey(priv, mappedKeyIdBuffer.releaseNonNull());
> 
> Why not move instead? and make webKitMediaClearKeyDecryptorFindAndSetKey
> receive a RefPtr<>&& ?

Done.

Thank you for the reviews, think we're in pretty good shape now!

[Charlie Turner]

Created attachment 358790 [details]
Patch

Address review comments

[Xabier Rodríguez Calvar]

Comment on attachment 358790 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:153
> -static gboolean webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate* priv, const WebCore::GstMappedBuffer& keyIDBuffer)
> +static gboolean webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate* priv, RefPtr<WebCore::GstMappedBuffer>&& keyIDBuffer)

Nit: I don't see a reason to change this from const & to &&, even when callers don't user this anymore. We are trasferring ownership of what is passed here and we don't need to.

Talking about the references, the RefPtr is ok, of course.

Other than this, the patch LGTM.

[Carlos Garcia Campos]

Comment on attachment 358790 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:153
>> +static gboolean webKitMediaClearKeyDecryptorFindAndSetKey(WebKitMediaClearKeyDecryptPrivate* priv, RefPtr<WebCore::GstMappedBuffer>&& keyIDBuffer)
> 
> Nit: I don't see a reason to change this from const & to &&, even when callers don't user this anymore. We are trasferring ownership of what is passed here and we don't need to.
> 
> Talking about the references, the RefPtr is ok, of course.
> 
> Other than this, the patch LGTM.

I suggested it because the reference was released when passed to this function, but I don't know why. Of course it's also fine to simply pass a const reference .

[Charlie Turner]

Created attachment 359026 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359026 [details]
Patch for landing

Rejecting attachment 359026 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 359026, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/10742194

[Charlie Turner]

Created attachment 359027 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359027 [details]
Patch for landing

Clearing flags on attachment: 359027

Committed r239921: <https://trac.webkit.org/changeset/239921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.