WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
216039
[MSE][GStreamer] Support Google Dynamic Ad Insertion (DAI)
https://bugs.webkit.org/show_bug.cgi?id=216039
Summary
[MSE][GStreamer] Support Google Dynamic Ad Insertion (DAI)
Enrique Ocaña
Reported
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Enrique Ocaña
Comment 1
2020-09-01 09:37:29 PDT
Created
attachment 407688
[details]
Patch
Philippe Normand
Comment 2
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?
Enrique Ocaña
Comment 3
2020-09-02 09:04:33 PDT
Created
attachment 407771
[details]
Patch
Alicia Boya García
Comment 4
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.
Enrique Ocaña
Comment 5
2020-09-04 09:56:08 PDT
Created
attachment 407986
[details]
Patch
Enrique Ocaña
Comment 6
2020-09-07 10:51:07 PDT
Created
attachment 408186
[details]
Patch
Enrique Ocaña
Comment 7
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.
Philippe Normand
Comment 8
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’
Enrique Ocaña
Comment 9
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&.
Enrique Ocaña
Comment 10
2020-09-08 04:24:58 PDT
Created
attachment 408223
[details]
Patch
Philippe Normand
Comment 11
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
Enrique Ocaña
Comment 12
2020-09-10 03:57:49 PDT
Created
attachment 408427
[details]
Patch
Enrique Ocaña
Comment 13
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.
EWS
Comment 14
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]
.
Radar WebKit Bug Importer
Comment 15
2020-09-10 05:45:23 PDT
<
rdar://problem/68638316
>
Enrique Ocaña
Comment 16
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.
Philippe Normand
Comment 17
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.
Philippe Normand
Comment 18
2022-05-22 09:16:30 PDT
Pull request:
https://github.com/WebKit/WebKit/pull/893
EWS
Comment 19
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug