Bug 189924 – [MSE][GStreamer] Use sentinel buffer to detect end of append

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

[Alicia Boya García]

This patch introduces a new mechanism to detect when an append has
been consumed completely by the demuxer. It takes advantage of the
fact that buffer pushing is synchronous: both the appsrc and the
demuxer live in the same streaming thread. When appsrc pushes a
buffer, it's actually making a qtdemux function call (it calls its
"chain" function). The demuxer will return from that call when it has
finished processing that buffer; only then the control returns to
appsrc, who can push the next buffer.

By pushing an additional buffer and capturing it in a probe we can
detect reliably when the previous buffer has been processed.
Because the pipeline only has one thread, at this point no more frames
can arrive to the appsink.

This replaces the old method of detecting end of append which relied
on the `need-data` event, which is more difficult to handle correctly
because it fires whenever the appsrc is empty (or below a given
level), which also happens when a buffer has not been pushed yet or
in response to a flush.

[Alicia Boya García]

Created attachment 350669 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Alicia Boya García from comment #0)
> This patch introduces a new mechanism to detect when an append has
> been consumed completely by the demuxer. It takes advantage of the
> fact that buffer pushing is synchronous: both the appsrc and the
> demuxer live in the same streaming thread. When appsrc pushes a
> buffer, it's actually making a qtdemux function call (it calls its
> "chain" function). The demuxer will return from that call when it has
> finished processing that buffer; only then the control returns to
> appsrc, who can push the next buffer.

Ok, I'll review this but I have one question. Can we guarantee that this is going to happen for other demuxers we require, say matroskademux for WebM? Cause we'd be screwed...

[Philippe Normand]

I think the same could be done with an event. No need for a GstMeta.

[Xabier Rodríguez Calvar]

Comment on attachment 350669 [details]
Patch

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

I am not against the idea of this patch, we should ensure that it is not going to backfire soon and that you should fix some stuff in it. Thibault's and Enrique's opinion are appreciated here.

Another question is which problems are you trying to fix with it.

> Source/WebCore/ChangeLog:12
> +        fact that buffer pushing is synchronous: both the appsrc and the
> +        demuxer live in the same streaming thread. When appsrc pushes a
> +        buffer, it's actually making a qtdemux function call (it calls its

As I said in my first comment, I can think this is true for qtdemux but I don't know how this is going to work for other demuxers like matroska's. Besides, this relies on the internal implementation of qtdemux which might change at some point, creating a lot of trouble in this case. Thibault, what do you think about this?

> Source/WebCore/ChangeLog:15
> +        appsrc, who can push the next buffer.

that can push the buffer

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:59
> +static GType endOfAppendMetaType = 0;
> +static const GstMetaInfo* webKitEndOfAppendMetaInfo = nullptr;

Even when they are not static members, my gut still tells me they should be prefixed with s_. If you have any doubts, I think we could even put all this (struct EndOfAppendMeta and GType definitions) inside AppendPipeline's class. It is integral part of its new way of working in the end, right?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:65
> +void AppendPipeline::staticInitialization()
> +{
> +    ASSERT(WTF::isMainThread());
> +    if (s_staticInitializationDone)
> +        return;

Where this is called, you should use std::call_once instead: https://en.cppreference.com/w/cpp/thread/call_once and avoid this manually.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:67
> +    const char* tags[] = {nullptr};

I don't know why but my gut tells me it should be  { nullprt }.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:140
> +    staticInitialization();

What I said about std::call_once goes here.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:798
> +    RELEASE_ASSERT(pushDataBufferRet == GST_FLOW_OK);

We should handle this more gracefully.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:802
> +    // Push an additional empty buffer that marks the end of the append.
> +    // This buffer is detected and consumed by appsrcEndOfAppendCheckerProbe(),
> +    // which uses it to signal the successful completion of the append.

I would appreciate here a bit more of elaboration as you did on the ChangeLog to explain the same thread, etc.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:809
> +    RELEASE_ASSERT(pushEndOfAppendBufferRet == GST_FLOW_OK);

Ditto.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #3)
> I think the same could be done with an event. No need for a GstMeta.

Also true.

[Enrique Ocaña]

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

Is there some use case that this patch solves but is broken with the already existing implementation?

This implementation relies on sharing the same streaming thread between all the pipeline elements. This hasn't always been the case, because the (asynchronous = multithread) EME decryptors were placed in the append pipeline at some point (even though now they have or are going to be moved to the playback pipeline).

As an extra safety measure, would it make sense to store the WTF::currentThead() in an AppendPipeline attribute in handleNewAppsinkSample() and check that currentThread() is the same one in appsrcEndOfAppendCheckerProbe()? If they're different, it means that the assumption of "same streaming thread in all the pipeline" no longer applies and we should runtime_assert.

I feared that this patch was sustained on a race condition but I was wrong. Let me explain: The concept of "end of append" means that all the possible samples that could have been generated in this append operation have been generated. You're detecting the end of the processing in the appsrc, but as the probe is called before the buffer goes on to the demuxer, some time must still pass (so, a race can happen) between the moment the probe detects the sentinel and the sentinel buffer finishes being processing in the demuxe... hmmm... wait... The sentinel buffer is discarded. And if it was captured by the probe it means that the previous "real" buffer processing (in the demuxer, etc.) has already finished (because of single threading), so the end of the append can actually be signaled (if single threading is guaranteed), so YOU ARE RIGHT. :-)

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-637
> -    if (m_appendState != AppendState::Ongoing && m_appendState != AppendState::Sampling) {

Why can't this condition happen anymore?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1036
> -        // No useful data, but notify anyway to complete the append operation.

Why is this removed? Would this hipotetical condition trigger an end-of-append and didReceiveAllPendingSamples() would have been called anyway in the end, so no need to call it here? (You can fake this hipotetical condition by commenting out one of the types (eg: audio or video) in parseDemuxerSrcPadCaps() so it gets detected as Unknown)

[Thibault Saunier]

Comment on attachment 350669 [details]
Patch

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

We discussed that approach with Alicia a while back and this was the best approach we came up with. Ideally appsrc would let us push serialized event and query and we could reuse that mechanism but it is not the case and not totally needed here.

>> Source/WebCore/ChangeLog:12
>> +        buffer, it's actually making a qtdemux function call (it calls its
> 
> As I said in my first comment, I can think this is true for qtdemux but I don't know how this is going to work for other demuxers like matroska's. Besides, this relies on the internal implementation of qtdemux which might change at some point, creating a lot of trouble in this case. Thibault, what do you think about this?

I do not see anything qtdemux specific in here no. The only thing that could screw that approach is to add a queue between the src and the demuxer which, well we are in control of afaict.

[Alicia Boya García]

(In reply to Philippe Normand from comment #3)
> I think the same could be done with an event. No need for a GstMeta.

This is something I have discussed in the past Multimedia Hackfest. Yes, a downstream in-band non-sticky event would work for this. But (and this is a big but), you need a way to ensure the event is sent *after* the buffer and this is impossible to do that with the current appsrc API, because although there is gst_app_src_push_buffer() and gst_app_src_push_sample(), as of writing there is no gst_app_src_push_event().

You may be tempted to attempt to workaround it like this:

pushNewBuffer(GstBuffer* buffer) {
    gst_app_src_push_buffer(appsrc, buffer);
    gst_pad_push_event(appsrc_pad, endOfAppendEvent);
}

But that won't work. To understand why, think carefully about what each of these functions do:

gst_app_src_push_buffer() adds the buffer to the appsrc internal queue and returns immediately. That's all there is to it as far as the calling thread is concerned (in this case the main thread).

At some point later, the streaming thread that was blocked in the appsrc loop function waiting for new buffers to arrive will notice it. It will remove the buffer from its internal queue, release the appsrc lock and call gst_pad_push(appsrc:src) with the new buffer, which in turn pass it to the peer pad using gst_pad_chain_data_unchecked(peer_pad) and will result in the chain function of the next element being called.

At that point the streaming thread is running code from the demuxer element. The demuxer may in turn create some new GstBuffers with some demuxed frames and call gst_pad_push() with them, and so on. A typical (very simplified) stack trace of the streaming thread would look like this:

gst_app_sink_chain            (appsink:sink,          parsed_frame)
gst_pad_chain_data_unchecked  (appsink:sink,          parsed_frame)
gst_pad_push                  (opusparse:src,         parsed_frame)
gst_opus_parse_chain          (opusparse:sink,        opus_frame)
gst_pad_chain_data_unchecked  (opusparse:sink,        opus_frame)
gst_pad_push                  (matroskademux:audio_0, opus_frame)
gst_matroska_demux_chain      (matroskademux:sink,    mkv_buffer)
gst_pad_chain_data_unchecked  (matroskademux:sink,    mkv_buffer)
gst_pad_push                  (appsrc:src,            mkv_buffer)
gst_app_src_loop              (appsrc:src)

(The actual stack trace is more complicated. Many of these functions don't exist with that name because they are actually implemented by base classes that call configurable functions of their child class... Still, this pseudo-stack trace is helpful for illustrating the control flow.)

Notice there is nothing fancy here. Pushing a buffer esentially amounts to calling a function of the downstream element. When the downstream element is done, it returns and the upstream element can continue. The streaming thread is running code of only one element at a time.

This does not mean that you could not call code of an element from the main thread... In fact, this is often done for operations that require waiting for the element to do something, like changing its state or sending flush events. How do elements manage to not explode in cases like this? That's what the element locks are for. In order to do something with an element, you need to acquire the element lock, so in these cases the main thread and the streaming thread take turns to borrow a certain element.

Let's give a look at events now. gst_pad_push_event(pad, event) receives an event, find the peer pad and calls gst_pad_send_event(peer_pad, event). This function, in turn, calls the "event" function specific of the element, which was defined with gst_pad_set_event_function() during the initialization of the pad. This is very similar to what was described before for gst_pad_send().

There are many types of events. Some travel downstream (e.g. SEGMENT, CAPS, EOS...) and some travel upstream (e.g. SEEK, QOS...). A few can travel either way, depending on whether they are received by a sink pad or source pad (currently that list includes only FLUSH_START, FLUSH_STOP and potentially custom events you create).

Our potential END_OF_APPEND event would travel only downstream.

Furthermore, downstream events are divided in two kinds: serialized (AKA "in-band") and non-serialized (AKA "out-of-band"). Serialized events travel the pipeline serialized with the buffers: that is, for any element downstream, a buffer pushed before the event is guaranteed to arrive before the event and a buffer pushed after the event is guaranteed to arrive after the event.

This serialization of buffers and in-band events pushed from potentially different threads (think the case where gst_pad_push_event() is called from the main thread) is achieved by the sinkpads' STREAM_LOCK. The stream lock of a pad is held during the gst_pad_chain_data_unchecked() or gst_pad_send_event() call. That is, until downstream of a pad has finished with a given buffer or serialized event, the pad will not accept more buffers or serialized events. (Further attempts will block.)

Most downstream events are in-band, with the notable exception of FLUSH_START. Our potential END_OF_APPEND event would also need to be in-band: we would not want it to overtake the data we wanted to demux in the pipeline.

Downstream events further divide into sticky and non-sticky. Sticky events are stored by the pad and can be later queried with gst_pad_get_sticky_event(). They are also resent if the pad is relinked. We are not interested in any of that, so the END_OF_APPEND would be non-sticky.

With all this understood... What would be the problem with calling gst_pad_push_event() from the main thread like in the pseudo code before?

1. If the buffer had already been pushed by appsrc, the attempt to acquire the stream lock of matroskademux' sinkpad would block the main thread while the demuxing occurs. Once matroskademux is done, the event would be sent and the gst_pad_push_event() call would return.

   Actually at this point the event would be of no use to us. We know the demuxer has finished processing the buffer with the stream lock alone. In effect, we would be making demuxing synchronous: even if the demuxer is running in another thread, we end up blocking the main thread waiting for it. Not ideal, definitely not what we wanted initially.

2. More worringly, there is a fatal race condition: If the streaming thread has not awaken and appsrc sent the buffer by the time we make the call (which is perfectly plausible), we will acquire the stream lock before the demuxer receives the buffer with the actual data. Therefore, we would be receiving the END_OF_APPEND event before the append has actually arrived the demuxer.

The only way to fix these problems with events would be to add support in appsrc to enqueue downstream on-band events *in the same queue* that it already uses for buffers. This would solve (1) because the event would be sent from the streaming thread without blocking the main thread for it and (2) because events and buffer would be stored as if they were the same thing and emitted in FIFO order.

Of course, adding event support to appsrc would not be an easy patch, partly because appsrc already emits some events (like SEGMENT, CAPS and EOS) -- so what should it do if the user enqueues one of these? It would require a new API function and a new GStreamer release.
Based on previous experience, that could delay this patch for months.

So, as a conclusion, yes, we could use events, but only after adding support for them in appsrc. Alternatively, instead of enqueing downstream in-band events that work like buffers we could... just use buffers. That is what this patch is about.

Should appsrc support event queing at some point, modifying AppendPipeline to use an event instead of a buffer would be trivial. In fact, apart from the semantics -- whether we call this thing "buffer" or "event", the mechanics using events would be exactly the same: instead of calling gst_app_src_push_buffer() you would call gst_app_src_push_event(), instead of creating a buffer with a GstMeta you would create a custom event with a GstStructure, instead of creating a probe that listens for a buffer containing a specific meta and drops it, you would create a probe that listens for an event containing a specific GstStructure and drops it.

[Alicia Boya García]

Comment on attachment 350669 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:59
>> +static const GstMetaInfo* webKitEndOfAppendMetaInfo = nullptr;
> 
> Even when they are not static members, my gut still tells me they should be prefixed with s_. If you have any doubts, I think we could even put all this (struct EndOfAppendMeta and GType definitions) inside AppendPipeline's class. It is integral part of its new way of working in the end, right?

I think it would be too surprising to use s_* for something that is not a static member of the class.

About making it a static member vs a static global variable... I don't know, they are esentially the same with the difference that members are scoped (which has little relevance in this case, since the whole AppendPipeline.cpp is all about AppendPipeline class) and that static members have to be exposed in the header file (which is undesirable for implementation details like this, but overall it's not a big problem anyway). I have no strong feelings towards neither approach; I'll go with the static member as suggested.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:65
>> +        return;
> 
> Where this is called, you should use std::call_once instead: https://en.cppreference.com/w/cpp/thread/call_once and avoid this manually.

Cool, I didn't know about it.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:798
>> +    RELEASE_ASSERT(pushDataBufferRet == GST_FLOW_OK);
> 
> We should handle this more gracefully.

The assert has a reason to be there. This can only fail if the appsrc is flushing or EOS. That should not happen unless there is a bug in AppendPipeline or another piece of our MSE implementation, therefore it makes sense to assert.

Notwithstanding that, I could use g_return_if_fail() instead of RELEASE_ASSERT(), so that should the assertion fail, WebKit does not crash but a CRITICAL is printed and captured with the debugger if necessary.

[Alicia Boya García]

(In reply to Xabier Rodríguez Calvar from comment #4)
> Another question is which problems are you trying to fix with it.

(In reply to Enrique Ocaña from comment #6)
> Is there some use case that this patch solves but is broken with the already
> existing implementation?

I want to get rid of complexity in the AppendPipeline. Eventually I want to get rid of the entire state machine, and all the special-cases in if()s. In the end, I want an AppendPipeline that I can understand and explain fully and use without fear of so many edge cases. I also want aborts to work as in the spec and do so reliably (currently blocked by https://bugzilla.gnome.org/show_bug.cgi?id=795424). I did that in the past in my work branch and now I'm trying to bring that upstream. This is a piece of that.

(In reply to Xabier Rodríguez Calvar from comment #4)
> As I said in my first comment, I can think this is true for qtdemux but I
> don't know how this is going to work for other demuxers like matroska's.
> Besides, this relies on the internal implementation of qtdemux which might
> change at some point, creating a lot of trouble in this case. Thibault, what
> do you think about this?

This is independent of the specific demuxer used. In relies just on how push mode scheduling works in GStreamer, which is extremely unlikely to change anytime soon (and should it happen, it would break all GStreamer).

(In reply to Enrique Ocaña from comment #6)
> This implementation relies on sharing the same streaming thread between all
> the pipeline elements. This hasn't always been the case, because the
> (asynchronous = multithread) EME decryptors were placed in the append
> pipeline at some point (even though now they have or are going to be moved
> to the playback pipeline).

This depends on being there only one streaming thread indeed. But so did the previous implementation. `need-data` fires when the appsrc is empty, not when the whole pipeline is empty. If it worked before with multiple streaming threads, I don't know how.

What I wrote in the previous mega-reply applies for queues too, but there is a small important detail: the chain method of a queue element stores the buffer in the queue and returns immediately. Then the next streaming thread notices it and continues on its own. Queues are special elements in that their sinkpad and srcpad live in different streaming threads. Most elements spawning threads are queues of some kind.

If at some point it becomes necessary to introduce a element that spawns a new streaming thread (hopefully not), it would be necessary to rethink this. In general terms, the end-of-append buffer/event would have to travel the queue and be detected and removed somewhere in the last streaming thread. A event is better in this case because it can travel easily through many elements (at least transform elements), but because of what I explained before, it would have to be pushed initially as a buffer so that it is correctly serialized; it can later be converted into an event in a probe. The actual challenges would be convincing the demuxers to forward the event to all the srcpads (neither qtdemux nor matroskademux do this currently) and handling the case where there are no srcpads yet. I'm not going down that route since I have no reason to do that as of now.

(In reply to Enrique Ocaña from comment #6)
> As an extra safety measure, would it make sense to store the
> WTF::currentThead() in an AppendPipeline attribute in
> handleNewAppsinkSample() and check that currentThread() is the same one in
> appsrcEndOfAppendCheckerProbe()? If they're different, it means that the
> assumption of "same streaming thread in all the pipeline" no longer applies
> and we should runtime_assert.

It can be done. It would add a little bit of noise to the code, but not too much. The only case it would fail would be if someone modified the pipeline to introduce a queue element or similar, in which case they would be warned of what they have broken.

(In reply to Enrique Ocaña from comment #6)
> > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-637
> > -    if (m_appendState != AppendState::Ongoing && m_appendState != AppendState::Sampling) {
> 
> Why can't this condition happen anymore?

Could it happen before? How? I've never seen it so far. I've run the YT tests after re-adding it just in case but found not a single instance.

(In reply to Enrique Ocaña from comment #6)
> > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1036
> > -        // No useful data, but notify anyway to complete the append operation.
> 
> Why is this removed? Would this hipotetical condition trigger an
> end-of-append and didReceiveAllPendingSamples() would have been called
> anyway in the end, so no need to call it here? (You can fake this
> hipotetical condition by commenting out one of the types (eg: audio or
> video) in parseDemuxerSrcPadCaps() so it gets detected as Unknown)

It has been removed because otherwise we would handle end-of-append twice. The end of append mechanism works exactly the same way regardless of whether what has been appended is an initialization segment, a media segment with a track of any type, or chunks or concatenations thereof. No need for special cases there.

BTW: The append is not guaranteed to end after the initialization segment, despite what the old code seemed to assume.

[Alicia Boya García]

Created attachment 350873 [details]
Patch

[Philippe Normand]

Comment on attachment 350873 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:159
> +    }, this, nullptr);

Would it be possible to pass a weak ptr here instead of `this`? The AppendPipeline could inherit from CanMakeWeakPtr.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:281
> +    GST_TRACE_OBJECT(this, "posting end-of-append request to bus");

s/this/m_pipeline.get()

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:617
> +    GST_TRACE_OBJECT(this, "received end-of-append");

s/this/m_pipeline.get()

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:793
> +    GST_TRACE_OBJECT(this, "pushing data buffer %p", buffer);

Nit: Use GST_PTR_FORMAT
Also, s/this/m_pipeline.get() :)

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:811
> +    GST_TRACE_OBJECT(this, "pushing end-of-append buffer %p", endOfAppendBuffer);

s/this/m_pipeline.get() and GST_PTR_FORMAT

[Enrique Ocaña]

Comment on attachment 350873 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:159
>> +    }, this, nullptr);
> 
> Would it be possible to pass a weak ptr here instead of `this`? The AppendPipeline could inherit from CanMakeWeakPtr.

But in the end, even if you use a WeakPtr, you still need to pass a pointer to the WeakPtr (not a WeakPtr full object). How would you manage the pointer lifecycle?

What we have done in the past with probes is to save the probe id as an AppendPipeline private attribute and then remove the probe in ~AppendPipeline(). IMHO, that's what should be done here.

[Alicia Boya García]

Comment on attachment 350873 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:159
>>> +    }, this, nullptr);
>> 
>> Would it be possible to pass a weak ptr here instead of `this`? The AppendPipeline could inherit from CanMakeWeakPtr.
> 
> But in the end, even if you use a WeakPtr, you still need to pass a pointer to the WeakPtr (not a WeakPtr full object). How would you manage the pointer lifecycle?
> 
> What we have done in the past with probes is to save the probe id as an AppendPipeline private attribute and then remove the probe in ~AppendPipeline(). IMHO, that's what should be done here.

By the time AppendPipeline is destroyed the pipeline has already transitioned to NULL state and has been unref'ed (and hence become impossible for the probe to be called). There is no need to use a weak pointer there.

Saving the probe id is unnecessary if you don't need to remove it later while the element is active. All the probes are tear down with the pad. Adding a probe will not increase the reference count of the pad, it will not leak.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:281
>> +    GST_TRACE_OBJECT(this, "posting end-of-append request to bus");
> 
> s/this/m_pipeline.get()

Why? It's actually the AppendPipeline doing its thing, not the boring pipeline. If I want to know which AppendPipeline is doing something it's usually more useful to know the AppendPipeline pointer than the GstPipeline object owned by it.

[Alicia Boya García]

Created attachment 350961 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350961 [details]
Patch

Clearing flags on attachment: 350961

Committed r236547: <https://trac.webkit.org/changeset/236547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Because that macro is intended to be used for GstObjects, not any kind of pointer

[Alicia Boya García]

(In reply to Philippe Normand from comment #18)
> Because that macro is intended to be used for GstObjects, not any kind of
> pointer

Hmmm... You are right, the documentation says "the GObject the message belongs to".

Then gst_debug_print_object() tries to guess what it is with GObject metadata and if it fails it returns just the pointer string.

This works, but theoretically the AppendPipeline object layout could end up in such a way it could be misunderstood for a certain GStreamer object.

[Thibault Saunier]

(In reply to Alicia Boya García from comment #19)
> (In reply to Philippe Normand from comment #18)
> > Because that macro is intended to be used for GstObjects, not any kind of
> > pointer
> 
> Hmmm... You are right, the documentation says "the GObject the message
> belongs to".

The doc is wrong, this also perfectly handle GstMiniObject :-)

> Then gst_debug_print_object() tries to guess what it is with GObject
> metadata and if it fails it returns just the pointer string.
> 
> This works, but theoretically the AppendPipeline object layout could end up
> in such a way it could be misunderstood for a certain GStreamer object.

In that case I would rather use a good Pipeline name and use `m_pipeline.get()` all around (I started to do that everywhere I put my handes actually :-))

[Alicia Boya García]

Amended in https://bugs.webkit.org/show_bug.cgi?id=190045

[Xabier Rodríguez Calvar]

Comment on attachment 350961 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:108
> +    // Used only for asserting that there is only one streaming thread.
> +    // Only the pointers are compared.
> +    WTF::Thread* m_streamingThread;

My preference would have been to keep the code related to check the streaming thread under #ifndef NDEBUG for debug builds

[Alicia Boya García]

(In reply to Xabier Rodríguez Calvar from comment #22)
> Comment on attachment 350961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350961&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:108
> > +    // Used only for asserting that there is only one streaming thread.
> > +    // Only the pointers are compared.
> > +    WTF::Thread* m_streamingThread;
> 
> My preference would have been to keep the code related to check the
> streaming thread under #ifndef NDEBUG for debug builds

I checked in advance that getting the thread is a very fast operation (after the first call, it just gets a pointer from thread local storage). Comparing pointers of course is also very fast.

[Michael Catanzaro]

Any substantial extra code needed for ASSERTS is usually guarded by #if !ASSERT_DISABLED.

[Alicia Boya García]

After discussion with Calvaris, we have decided to keep the check, but make it not a release assert.

[Alicia Boya García]

Created attachment 351276 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351276 [details]
Patch

Clearing flags on attachment: 351276

Committed r236717: <https://trac.webkit.org/changeset/236717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.