Bug 216039 - [MSE][GStreamer] Support Google Dynamic Ad Insertion (DAI)
Summary: [MSE][GStreamer] Support Google Dynamic Ad Insertion (DAI)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-01 09:24 PDT by Enrique Ocaña
Modified: 2022-05-25 01:01 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.75 KB, patch)
2020-09-01 09:37 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (6.06 KB, patch)
2020-09-02 09:04 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2020-09-04 09:56 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (7.48 KB, patch)
2020-09-07 10:51 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2020-09-08 04:24 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2020-09-10 03:57 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2020-09-01 09:24:55 PDT
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
Comment 1 Enrique Ocaña 2020-09-01 09:37:29 PDT
Created attachment 407688 [details]
Patch
Comment 2 Philippe Normand 2020-09-02 07:35:39 PDT
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?
Comment 3 Enrique Ocaña 2020-09-02 09:04:33 PDT
Created attachment 407771 [details]
Patch
Comment 4 Alicia Boya García 2020-09-02 09:37:20 PDT
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.
Comment 5 Enrique Ocaña 2020-09-04 09:56:08 PDT
Created attachment 407986 [details]
Patch
Comment 6 Enrique Ocaña 2020-09-07 10:51:07 PDT
Created attachment 408186 [details]
Patch
Comment 7 Enrique Ocaña 2020-09-07 11:01:18 PDT
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.
Comment 8 Philippe Normand 2020-09-08 01:31:58 PDT
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’
Comment 9 Enrique Ocaña 2020-09-08 02:18:59 PDT
(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&.
Comment 10 Enrique Ocaña 2020-09-08 04:24:58 PDT
Created attachment 408223 [details]
Patch
Comment 11 Philippe Normand 2020-09-08 05:02:27 PDT
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
Comment 12 Enrique Ocaña 2020-09-10 03:57:49 PDT
Created attachment 408427 [details]
Patch
Comment 13 Enrique Ocaña 2020-09-10 04:04:59 PDT
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.
Comment 14 EWS 2020-09-10 05:44:15 PDT
Committed r266820: <https://trac.webkit.org/changeset/266820>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408427 [details].
Comment 15 Radar WebKit Bug Importer 2020-09-10 05:45:23 PDT
<rdar://problem/68638316>
Comment 16 Enrique Ocaña 2022-05-09 09:47:03 PDT
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.
Comment 17 Philippe Normand 2022-05-21 09:23:38 PDT
(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.
Comment 18 Philippe Normand 2022-05-22 09:16:30 PDT
Pull request: https://github.com/WebKit/WebKit/pull/893
Comment 19 EWS 2022-05-25 01:01:13 PDT
Committed r294791 (250950@main): <https://commits.webkit.org/250950@main>

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