[webkit-reviews] review denied: [Bug 189924] [MSE][GStreamer] Use sentinel buffer to detect end of append : [Attachment 350669] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 189924: [MSE][GStreamer] Use sentinel buffer to detect end of append
https://bugs.webkit.org/show_bug.cgi?id=189924

Attachment 350669: Patch

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




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 350669
  --> https://bugs.webkit.org/attachment.cgi?id=350669
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.