WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(62.29 KB, patch)
2021-08-19 09:03 PDT
,
Alicia Boya García
no flags
Details
Formatted Diff
Diff
Patch
(62.24 KB, patch)
2021-08-23 01:08 PDT
,
Alicia Boya García
no flags
Details
Formatted Diff
Diff
Patch
(62.25 KB, patch)
2021-08-23 04:12 PDT
,
Alicia Boya García
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Alicia Boya García
Comment 1
2021-08-13 07:16:52 PDT
Created
attachment 435480
[details]
Patch
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
Created
attachment 435866
[details]
Patch
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
Created
attachment 436159
[details]
Patch
Alicia Boya García
Comment 8
2021-08-23 04:12:49 PDT
Created
attachment 436181
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug