RESOLVED FIXED 229072
[MSE][GStreamer] Implement multi-track support
https://bugs.webkit.org/show_bug.cgi?id=229072
Summary [MSE][GStreamer] Implement multi-track support
Alicia Boya García
Reported 2021-08-13 07:15:38 PDT
This patch adds support for SourceBuffer having more than one track in the GStreamer port. This fixes the following LayoutTests: imported/w3c/web-platform-tests/media-source/mediasource-activesourcebuffers.html media/media-source/media-source-has-audio-video.html media/media-source/only-bcp47-language-tags-accepted-as-valid.html
Attachments
Patch (61.90 KB, patch)
2021-08-13 07:16 PDT, Alicia Boya García
no flags
Patch (62.29 KB, patch)
2021-08-19 09:03 PDT, Alicia Boya García
no flags
Patch (62.24 KB, patch)
2021-08-23 01:08 PDT, Alicia Boya García
no flags
Patch (62.25 KB, patch)
2021-08-23 04:12 PDT, Alicia Boya García
no flags
Alicia Boya García
Comment 1 2021-08-13 07:16:52 PDT
Xabier Rodríguez Calvar
Comment 2 2021-08-13 08:52:10 PDT
Comment on attachment 435480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435480&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:366 > + m_done = true; TRUE > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:293 > +void AppendPipeline::parseDemuxerSrcPadCaps(GstCaps* demuxerSrcPadCaps, GRefPtr<GstCaps>* dstParsedCaps, > + StreamType* dstStreamType, FloatSize* dstPresentationSize) if dst* parameters are output parameters, please return a tuple. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:634 > +AppendPipeline::Track* AppendPipeline::tryCreateTrackFromPad(GstPad* demuxerSrcPad, bool* dstAppendParsingFailed, int trackIndex) Please return an optional and avoid the failed bool
Alicia Boya García
Comment 3 2021-08-18 11:52:50 PDT
Comment on attachment 435480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435480&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:634 >> +AppendPipeline::Track* AppendPipeline::tryCreateTrackFromPad(GstPad* demuxerSrcPad, bool* dstAppendParsingFailed, int trackIndex) > > Please return an optional and avoid the failed bool An optional is not the right choice because null is a valid return value in this case.
Alicia Boya García
Comment 4 2021-08-19 09:03:39 PDT
Xabier Rodríguez Calvar
Comment 5 2021-08-20 02:40:35 PDT
Comment on attachment 435866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435866&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:366 > + m_done = true; Nit: TRUE > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:296 > + GRefPtr<GstCaps> parsedCaps = GRefPtr<GstCaps>(demuxerSrcPadCaps); Do you need to explicitly call the constructor? It should be called automatically, right? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:298 > + FloatSize presentationSize = FloatSize(); Isn't no parameters default constructor being called here as well? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:314 > + return { parsedCaps, streamType, presentationSize }; Please, WTFMove the objects into the tuple. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:489 > + GST_DEBUG("Notifying SourceBuffer of initialization segement."); *segment > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:699 > + GST_WARNING_OBJECT(pipeline(), "Couldn't find a matching pre-existing track for pad '%s' with parsed caps %" GST_PTR_FORMAT " on non-first initialization segment, will be connected to a black hole probe.", GST_PAD_NAME(demuxerSrcPad), parsedCaps.get()); Huge line :) > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:705 > +void AppendPipeline::linkPadWithTrack(GstPad *demuxerSrcPad, Track &track) GstPad* demuxerSrcPad, Track& track > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:830 > +const char* AppendPipeline::streamTypeToString(StreamType streamType) This is breaking the build > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:112 > + static std::tuple<GRefPtr<GstCaps>, AppendPipeline::StreamType, FloatSize> parseDemuxerSrcPadCaps(GstCaps* demuxerSrcPadCaps); > + Ref<WebCore::TrackPrivateBase> makeWebKitTrack(int trackIndex); Think if you can remove paremeter names, plz. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:126 > + std::pair<CreateTrackResult, AppendPipeline::Track*> tryCreateTrackFromPad(GstPad* demuxerSrcPad, int padIndex); > + AppendPipeline::Track* tryMatchPadToExistingTrack(GstPad* demuxerSrcPad); > + void linkPadWithTrack(GstPad* demuxerSrcPad, Track&); Think if you can remove paremeter names, plz.
Alicia Boya García
Comment 6 2021-08-22 13:22:38 PDT
Comment on attachment 435866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435866&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:112 >> + Ref<WebCore::TrackPrivateBase> makeWebKitTrack(int trackIndex); > > Think if you can remove paremeter names, plz. It makes sense in the first case, since the name of the member function gives it away, but I would leave it in the other cases, because they are not necessarily obvious.
Alicia Boya García
Comment 7 2021-08-23 01:08:09 PDT
Alicia Boya García
Comment 8 2021-08-23 04:12:49 PDT
EWS
Comment 9 2021-08-23 06:22:38 PDT
Committed r281440 (240823@main): <https://commits.webkit.org/240823@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436181 [details].
Note You need to log in before you can comment on or make changes to this bug.