Bug 216039 – [MSE][GStreamer] Support Google Dynamic Ad Insertion (DAI)

• Component: Media
• Status: RESOLVED FIXED

[Enrique Ocaña]

DAI inserts advertisements in the middle of a regular MSE video by appending the ad video data and letting it be played. Then, the regular video data continues being appended and played.

What happens on the DAI transitions between regular and advertisement videos (and back) on MP4 is that each track has a different stream id, so qtdemux doesn't reuse streams and created a new pad/stream/track for the new piece of video. Our current code only supports a single simultaneous track per SourceBuffer, so it "disables" the second (new) one by attaching a black hole probe which drops all the buffers. Right after the new pad is created, the old pad/stream/track is removed, so in the end only a single "disabled" track remains.

For more info and test cases about Google DAI, have a look at:
https://developers.google.com/interactive-media-ads/docs/sdks/html5/dai
https://github.com/googleads/googleads-ima-html5-dai

[Enrique Ocaña]

Created attachment 407688 [details]
Patch

[Philippe Normand]

Comment on attachment 407688 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:221
> +        if (appendPipeline->m_errorReceived || appendPipeline->m_demux->numpads > 1)

s/numpads/numsrcpads ?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:841
> +        while (!done) {
> +            switch (gst_iterator_next(iterator.get(), &item)) {
> +            case GST_ITERATOR_OK: {
> +                remainingPad = GST_PAD(g_value_get_object(&item));
> +                done = true;
> +                g_value_reset(&item);
> +                break;
> +            }
> +            case GST_ITERATOR_RESYNC:
> +                gst_iterator_resync(iterator.get());
> +                break;
> +            case GST_ITERATOR_ERROR:
> +            case GST_ITERATOR_DONE:
> +                done = true;
> +                break;
> +            }
> +        }
> +        g_value_unset(&item);

Might be simpler to access the first src pad through m_demux.get()->srcpads which is a GList?

[Enrique Ocaña]

Created attachment 407771 [details]
Patch

[Alicia Boya García]

Comment on attachment 407688 [details]
Patch

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

Informal r+ with nits.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:820
> +        GUniquePtr<GstIterator> iterator(gst_element_iterate_src_pads(m_demux.get()));

You don't need all this iterator code. You know there is only one srcpad (above) and the list of pads is not going to change while you're iterating it inside the synchronous pad-added handler. You can replace the whole thing with:

GstPad* remainingPad = GST_PAD(m_demux.get().srcpads->data);

And because you are not going to remove it from the element, there is no need to refcount it via GRefPtr<GstPad>.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:846
> +            while (oldPeerPad && gst_pad_is_linked(oldPeerPad.get())) {

I can't think of a situation where oldPeerPad becomes null, element would have to be a source or muxer element.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:853
> +            if (oldPeerPad) {

Same. I would ASSERT(oldPeerPad); instead.

A comment about how this is fine because because pad removal and addition occur synchronously before samples flow would be nice, since it may not be obvious to the reader.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:856
> +                gst_pad_unlink(demuxerSrcPad, oldPeerPad.get());

oldPeerPad is not linked at this point. In fact, it not being linked is the exit condition of the loop above.

[Enrique Ocaña]

Created attachment 407986 [details]
Patch

[Enrique Ocaña]

Created attachment 408186 [details]
Patch

[Enrique Ocaña]

The previous patch caused a regression on media/media-source/media-source-seek-detach-crash.html.

The new version I've just submitted checks if the caps of the remaining qtdemux pad and the next element (parser/appsink) are compatible (have the same main media type), avoiding reconnection if they aren't.

With this last change there are no regressions in the media-source tests.

[Philippe Normand]

EWS is red

FAILED: Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/gstreamer/mse/AppendPipeline.cpp.o 
/usr/bin/ccache /usr/bin/c++  -DBUILDING_GTK__=1 -DBUILDING_WEBKIT -DBUILDING_WITH_CMAKE=1 -DBUILDING_WebCore -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DSTATICALLY_LINKED_WITH_PAL=1 -DSVN_REVISION=\"r266702\" -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -I../../Source/WebCore/platform/graphics/libwpe -I. -IDerivedSources/WebCore -I../../Source/WebCore -I../../Source/WebCore/Modules/airplay -I../../Source/WebCore/Modules/applepay -I../../Source/WebCore/Modules/applepay/paymentrequest -I../../Source/WebCore/Modules/applicationmanifest -I../../Source/WebCore/Modules/async-clipboard -I../../Source/WebCore/Modules/beacon -I../../Source/WebCore/Modules/cache -I../../Source/WebCore/Modules/credentialmanagement -I../../Source/WebCore/Modules/encryptedmedia -I../../Source/WebCore/Modules/encryptedmedia/legacy -I../../Source/WebCore/Modules/entriesapi -I../../Source/WebCore/Modules/fetch -I../../Source/WebCore/Modules/geolocation -I../../Source/WebCore/Modules/highlight -I../../Source/WebCore/Modules/indexeddb -I../../Source/WebCore/Modules/indexeddb/client -I../../Source/WebCore/Modules/indexeddb/server -I../../Source/WebCore/Modules/indexeddb/shared -I../../Source/WebCore/Modules/mediacapabilities -I../../Source/WebCore/Modules/mediacontrols -I../../Source/WebCore/Modules/mediarecorder -I../../Source/WebCore/Modules/mediasession -I../../Source/WebCore/Modules/mediasource -I../../Source/WebCore/Modules/mediastream -I../../Source/WebCore/Modules/mediastream/libwebrtc -I../../Source/WebCore/Modules/navigatorcontentutils -I../../Source/WebCore/Modules/notifications -I../../Source/WebCore/Modules/paymentrequest -I../../Source/WebCore/Modules/pictureinpicture -I../../Source/WebCore/Modules/plugins -I../../Source/WebCore/Modules/quota -I../../Source/WebCore/Modules/remoteplayback -I../../Source/WebCore/Modules/speech -I../../Source/WebCore/Modules/streams -I../../Source/WebCore/Modules/webaudio -I../../Source/WebCore/Modules/webauthn -I../../Source/WebCore/Modules/webauthn/cbor -I../../Source/WebCore/Modules/webauthn/fido -I../../Source/WebCore/Modules/webdatabase -I../../Source/WebCore/Modules/webdriver -I../../Source/WebCore/Modules/webgpu -I../../Source/WebCore/Modules/webgpu/WHLSL -I../../Source/WebCore/Modules/webgpu/WHLSL/AST -I../../Source/WebCore/Modules/websockets -I../../Source/WebCore/Modules/webxr -I../../Source/WebCore/accessibility -I../../Source/WebCore/accessibility/isolatedtree -I../../Source/WebCore/animation -I../../Source/WebCore/bindings -I../../Source/WebCore/bindings/js -I../../Source/WebCore/bridge -I../../Source/WebCore/bridge/c -I../../Source/WebCore/bridge/jsc -I../../Source/WebCore/contentextensions -I../../Source/WebCore/crypto -I../../Source/WebCore/crypto/algorithms -I../../Source/WebCore/crypto/keys -I../../Source/WebCore/crypto/parameters -I../../Source/WebCore/css -I../../Source/WebCore/css/parser -I../../Source/WebCore/css/typedom -I../../Source/WebCore/cssjit -I../../Source/WebCore/dom -I../../Source/WebCore/dom/messageports -I../../Source/WebCore/domjit -I../../Source/WebCore/editing -I../../Source/WebCore/fileapi -I../../Source/WebCore/history -I../../Source/WebCore/html -I../../Source/WebCore/html/canvas -I../../Source/WebCore/html/forms -I../../Source/WebCore/html/parser -I../../Source/WebCore/html/shadow -I../../Source/WebCore/html/track -I../../Source/WebCore/inspector -I../../Source/WebCore/inspector/agents -I../../Source/WebCore/inspector/agents/page -I../../Source/WebCore/inspector/agents/worker -I../../Source/WebCore/layout -I../../Source/WebCore/layout/blockformatting -I../../Source/WebCore/layout/blockformatting/tablewrapper -I../../Source/WebCore/layout/displaytree -I../../Source/WebCore/layout/floats -I../../Source/WebCore/layout/inlineformatting -I../../Source/WebCore/layout/inlineformatting/text -I../../Source/WebCore/layout/integration -I../../Source/WebCore/layout/invalidation -I../../Source/WebCore/layout/layouttree -I../../Source/WebCore/layout/tableformatting -I../../Source/WebCore/loader -I../../Source/WebCore/loader/appcache -I../../Source/WebCore/loader/archive -I../../Source/WebCore/loader/archive/mhtml -I../../Source/WebCore/loader/cache -I../../Source/WebCore/loader/icon -I../../Source/WebCore/mathml -I../../Source/WebCore/page -I../../Source/WebCore/page/animation -I../../Source/WebCore/page/csp -I../../Source/WebCore/page/scrolling -I../../Source/WebCore/platform -I../../Source/WebCore/platform/animation -I../../Source/WebCore/platform/audio -I../../Source/WebCore/platform/encryptedmedia -I../../Source/WebCore/platform/gamepad -I../../Source/WebCore/platform/graphics -I../../Source/WebCore/platform/graphics/cpu/arm -I../../Source/WebCore/platform/graphics/cpu/arm/filters -I../../Source/WebCore/platform/graphics/displaylists -I../../Source/WebCore/platform/graphics/filters -I../../Source/WebCore/platform/graphics/iso -I../../Source/WebCore/platform/graphics/opentype -I../../Source/WebCore/platform/graphics/transforms -I../../Source/WebCore/platform/mediacapabilities -I../../Source/WebCore/platform/mediarecorder -I../../Source/WebCore/platform/mediasession -I../../Source/WebCore/platform/mediastream -I../../Source/WebCore/platform/mediastream/libwebrtc -I../../Source/WebCore/platform/mock -I../../Source/WebCore/platform/mock/mediasource -I../../Source/WebCore/platform/network -I../../Source/WebCore/platform/sql -I../../Source/WebCore/platform/text -I../../Source/WebCore/platform/xr -I../../Source/WebCore/plugins -I../../Source/WebCore/rendering -I../../Source/WebCore/rendering/line -I../../Source/WebCore/rendering/mathml -I../../Source/WebCore/rendering/shapes -I../../Source/WebCore/rendering/style -I../../Source/WebCore/rendering/svg -I../../Source/WebCore/rendering/updating -I../../Source/WebCore/replay -I../../Source/WebCore/storage -I../../Source/WebCore/style -I../../Source/WebCore/svg -I../../Source/WebCore/svg/animation -I../../Source/WebCore/svg/graphics -I../../Source/WebCore/svg/graphics/filters -I../../Source/WebCore/svg/properties -I../../Source/WebCore/websockets -I../../Source/WebCore/workers -I../../Source/WebCore/workers/service -I../../Source/WebCore/workers/service/context -I../../Source/WebCore/workers/service/server -I../../Source/WebCore/worklets -I../../Source/WebCore/xml -I../../Source/WebCore/xml/parser -I../../Source/WebCore/Modules/gamepad -I../../Source/WebCore/platform/graphics/gpu -I../../Source/ThirdParty/xdgmime/src -I../../Source/WebCore/platform/graphics/cairo -I../../Source/WebCore/platform/graphics/freetype -I../../Source/WebCore/platform/graphics/harfbuzz -I../../Source/WebCore/platform/graphics/harfbuzz/ng -I../../Source/WebCore/platform/graphics/gstreamer -I../../Source/WebCore/platform/graphics/gstreamer/mse -I../../Source/WebCore/platform/graphics/gstreamer/eme -I../../Source/WebCore/platform/audio/gstreamer -I../../Source/WebCore/platform/encryptedmedia/clearkey -I../../Source/WebCore/platform/image-decoders -I../../Source/WebCore/platform/image-decoders/bmp -I../../Source/WebCore/platform/image-decoders/gif -I../../Source/WebCore/platform/image-decoders/ico -I../../Source/WebCore/platform/image-decoders/jpeg -I../../Source/WebCore/platform/image-decoders/jpeg2000 -I../../Source/WebCore/platform/image-decoders/png -I../../Source/WebCore/platform/image-decoders/webp -I../../Source/WebCore/platform/network/soup -I../../Source/WebCore/platform/graphics/texmap -I../../Source/WebCore/platform/graphics/nicosia -I../../Source/WebCore/page/scrolling/nicosia -I../../Source/WebCore/platform/graphics/texmap/coordinated -I../../Source/WebCore/platform/graphics/nicosia/cairo -I../../Source/WebCore/platform/graphics/nicosia/texmap -I../../Source/WebCore/accessibility/atk -I../../Source/WebCore/editing/atk -I../../Source/WebCore/page/gtk -I../../Source/WebCore/platform/adwaita -I../../Source/WebCore/platform/generic -I../../Source/WebCore/platform/gtk -I../../Source/WebCore/platform/graphics/egl -I../../Source/WebCore/platform/graphics/glx -I../../Source/WebCore/platform/graphics/gtk -I../../Source/WebCore/platform/graphics/opengl -I../../Source/WebCore/platform/graphics/wayland -I../../Source/WebCore/platform/graphics/x11 -I../../Source/WebCore/platform/mediastream/gtk -I../../Source/WebCore/platform/mediastream/gstreamer -I../../Source/WebCore/platform/network/gtk -I../../Source/WebCore/platform/text/gtk -IDerivedSources/ForwardingHeaders -isystem ../../Source/ThirdParty/libwebrtc/Source -isystem ../../Source/ThirdParty/libwebrtc/Source/webrtc -isystem ../../Source/ThirdParty/libwebrtc/Source/third_party/abseil-cpp -isystem /usr/include/gstreamer-1.0 -isystem /usr/include/glib-2.0 -isystem /usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem /usr/lib/x86_64-linux-gnu/libffi-3.2.1/include -isystem /usr/include/orc-0.4 -isystem /usr/lib/x86_64-linux-gnu/gstreamer-1.0/include -isystem /usr/include/libsoup-2.4 -isystem /usr/include/atk-1.0 -isystem /usr/include/enchant-2 -isystem /usr/include/gio-unix-2.0 -isystem /usr/include/libmount -isystem /usr/include/blkid -isystem /usr/include/libsecret-1 -isystem /usr/include/libxml2 -isystem /usr/include/gtk-3.0 -isystem /usr/include/pango-1.0 -isystem /usr/include/harfbuzz -isystem /usr/include/fribidi -isystem /usr/include/freetype2 -isystem /usr/include/libpng16 -isystem /usr/include/cairo -isystem /usr/include/pixman-1 -isystem /usr/include/gdk-pixbuf-2.0 -isystem /usr/include/at-spi2-atk/2.0 -isystem /usr/include/at-spi-2.0 -isystem /usr/include/dbus-1.0 -isystem /usr/lib/x86_64-linux-gnu/dbus-1.0/include -isystem /usr/include/openjpeg-2.3 -isystem /usr/include/wpe-1.0 -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare  -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIC   -pthread -std=c++17 -MD -MT Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/gstreamer/mse/AppendPipeline.cpp.o -MF Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/gstreamer/mse/AppendPipeline.cpp.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/gstreamer/mse/AppendPipeline.cpp.o -c ../../Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp
../../Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp: In member function ‘void WebCore::AppendPipeline::disconnectDemuxerSrcPadFromAppsinkFromAnyThread(GstPad*)’:
../../Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:851:42: error: base operand of ‘->’ has non-pointer type ‘WebCore::SourceBufferPrivateGStreamer’

[Enrique Ocaña]

(In reply to Philippe Normand from comment #8)

> EWS is red

I was using an outdated trunk that didn't have the patch from https://bugs.webkit.org/show_bug.cgi?id=214345, which changed m_sourceBufferPrivate from Ref<SourceBufferPrivateGStreamer> to SourceBufferPrivateGStreamer&.

[Enrique Ocaña]

Created attachment 408223 [details]
Patch

[Philippe Normand]

Comment on attachment 408223 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:818
> +            GRefPtr<GstPad> oldPeerPad = adoptGRef(gst_element_get_static_pad(m_appsink.get(), "sink"));

nit: you could use auto in all places where the variable type can be deduced by the compiler, like here.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:838
> +                oldPeerPadType = gst_structure_get_name(oldPeerPadCapsStructure);

indentation is off here

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:843
> +                remainingPadType = gst_structure_get_name(remainingPadCapsStructure);

ditto

[Enrique Ocaña]

Created attachment 408427 [details]
Patch

[Enrique Ocaña]

Comment on attachment 408223 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:818
>> +            GRefPtr<GstPad> oldPeerPad = adoptGRef(gst_element_get_static_pad(m_appsink.get(), "sink"));
> 
> nit: you could use auto in all places where the variable type can be deduced by the compiler, like here.

As a human developer reading code (instead of as a compiler building it) it's harder for me to deduce the type of some variables if they are auto. Still, I've changed the types as you suggest.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:838
>> +                oldPeerPadType = gst_structure_get_name(oldPeerPadCapsStructure);
> 
> indentation is off here

Thanks! I had missed it completely.

[EWS]

Committed r266820: <https://trac.webkit.org/changeset/266820>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408427 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68638316>

[Enrique Ocaña]

Unfortunately, after a recent AppendPipeline refactor, Google DAI is apparently not working anymore.

When using https://github.com/googleads/googleads-ima-html5-dai/blob/main/hls_js/simple/dai.html as a test case (properly served from a local web server with a git checkout), the video doesn't change from the first stream to the next one.

[Philippe Normand]

(In reply to Enrique Ocaña from comment #16)
> Unfortunately, after a recent AppendPipeline refactor, Google DAI is
> apparently not working anymore.
> 
> When using
> https://github.com/googleads/googleads-ima-html5-dai/blob/main/hls_js/simple/
> dai.html as a test case (properly served from a local web server with a git
> checkout), the video doesn't change from the first stream to the next one.

I started a patch. Most likely this broke when multi-track support was landed, ~9 months ago. We'll need a test otherwise it will break again.

[Philippe Normand]

Pull request: https://github.com/WebKit/WebKit/pull/893

[EWS]

Committed r294791 (250950@main): <https://commits.webkit.org/250950@main>

Reviewed commits have been landed. Closing PR #893 and removing active labels.