| Summary: | [MSE][GStreamer] WebKitMediaSrc rework | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Alicia Boya García <aboya> |
| Component: | WebKitGTK | Assignee: | Alicia Boya García <aboya> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, commit-queue, cturner, eric.carlson, ews-watchlist, mcatanzaro, pnormand, rniwa, tsaunier |
| Priority: | P2 | ||
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://github.com/web-platform-tests/wpt/pull/17801 | ||
| Attachments: | |||
|
Description
Alicia Boya García
2019-07-11 12:55:23 PDT
Created attachment 374003 [details]
Patch
Created attachment 374004 [details]
Patch
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/17801 Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review > Source/WebCore/platform/DataMutex.h:25 > +namespace WebCore { what's this DataMutex? If it's really needed, I think it should go in WTF and include unit tests > Source/WebCore/platform/MainThreadData.h:26 > +namespace WebCore { Ditto, this belongs to WTF I would say. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:703 > + RefPtr<Type##TrackPrivateGStreamer> track = Type##TrackPrivateGStreamer::create(makeWeakPtr(*this), i, stream); \ auto track = > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:784 > + GstStream* stream = gst_stream_collection_get_stream(m_streamCollection.get(), index); auto* > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:57 > + WebCore::MediaSourceStreamTypeGStreamer streamType() { return m_streamType; } isn't this inside WebCore namespace? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:45 > +using WebCore::DataMutex; > +using WebCore::MainThreadData; > +using WebCore::SourceBufferPrivateClient; why not using namespace WebCore? because of unified builds? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:98 > +G_DEFINE_TYPE_WITH_CODE(WebKitMediaSrc, webkit_media_src, GST_TYPE_ELEMENT, Use WEBKIT_DEFINE_TYPE *** Bug 182531 has been marked as a duplicate of this bug. *** Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:159 > + bool flushing { false }; These bools could be packed together with the ones above. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:166 > + while (frontIter != queue.end() && !GST_IS_SAMPLE(frontIter->get())) The raw loops may be better written with the standard find_if algorithm. It would be nicer if we had an invariant that everything in the queue is a sample, but I've not read the call-sites to know whether this is possible. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:173 > + while (!GST_IS_SAMPLE(backIter->get())) Ditto > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:189 > + bool removed { false }; Ditto about packing, can stick this bool above the pointer. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:197 > + GRefPtr<GstElement> parentElement = adoptGRef(GST_ELEMENT(gst_element_get_parent(element.get()))); I don't believe we need all the refcounting churn in this loop, do we? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:215 > + "Source", "Feeds sample coming from WebKit", "Igalia <aboya@igalia.com>"); Feeds samples from WebKit's MSE > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:259 > + GRefPtr<GstStreamCollection> newCollection = adoptGRef(gst_stream_collection_new(collection->upstream_id)); Why do we need to create new collection and not just append to the end? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:263 > + GRefPtr<GstStream> oldStream = gst_stream_collection_get_stream(collection, i); Doubt we need the refcounting > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:656 > +void webKitMediaSrcSeek(WebKitMediaSrc* source, guint64 startTime, double rate) <3 <3 Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/timeout_on_seek.py:9 > + else: Nit: I don’t write a lot of Python so I don’t know if it is idiomatic, but this else isn’t necessary Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review >> Source/WebCore/platform/DataMutex.h:25 >> +namespace WebCore { > > what's this DataMutex? If it's really needed, I think it should go in WTF and include unit tests Looks like a class that guards access to m_data using a mutex. Useful. Definitely belongs in WTF and merits review by Apple maintainers. We'll need to bikeshed about how the class is named with Apple (probably should say "Lock" instead of "Mutex") and make sure there's not already some equivalent class (I don't think so, but maybe I'm missing one). It's also a little weird that it locks a mutex that it doesn't own itself, so maybe LockHolder would be a more appropriate name... except of course we already have a LockHolder, and it's implemented using that LockHolder. Might be beneficial to first add this in a separate WTF patch, to make review easier. > Source/WebCore/platform/DataMutex.h:36 > + class LockedWrapper { Is it really necessary for the wrapper class to be public? > Source/WebCore/platform/DataMutex.h:38 > + LockedWrapper(DataMutex& dataMutex) explicit? Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review >>> Source/WebCore/platform/DataMutex.h:25 >>> +namespace WebCore { >> >> what's this DataMutex? If it's really needed, I think it should go in WTF and include unit tests > > Looks like a class that guards access to m_data using a mutex. Useful. Definitely belongs in WTF and merits review by Apple maintainers. We'll need to bikeshed about how the class is named with Apple (probably should say "Lock" instead of "Mutex") and make sure there's not already some equivalent class (I don't think so, but maybe I'm missing one). It's also a little weird that it locks a mutex that it doesn't own itself, so maybe LockHolder would be a more appropriate name... except of course we already have a LockHolder, and it's implemented using that LockHolder. > > Might be beneficial to first add this in a separate WTF patch, to make review easier. Yes, it's a class to guard access to members of the wrapped class behind a mutex. So you don't forget to take it before accessing that field! Not only that, but it also makes it self-documenting that these fields have to be accessed while locked, while members outside don't need it. Look for "m_streamingMembers" for an example of its usage. The name it's taken from Gecko, who has a very similar structure with the same purpose and name. https://lists.mozilla.org/pipermail/dev-platform/2019-January/023375.html That doesn't mean that we have to use the same one, but it's a start. It's short and memorable. I also thought "ProtectedData", but that would be a very confusing name to be anywhere close to the EME codebase. And it owns its own mutex (m_mutex). You can still get access to it to do finer-grained unlocking, it doesn't prevent that (and this is not Rust anyway), but at least you have to make an effort to actually use it wrong. >> Source/WebCore/platform/DataMutex.h:36 >> + class LockedWrapper { > > Is it really necessary for the wrapper class to be public? Yes, that's how the members of the wrapped object are accessed. Look for examples below. DataMutex::LockedWrapper is a RAII class that holds the mutex and lets you access the data, while DataMutex is the class that stores the data as its contents. >> Source/WebCore/platform/DataMutex.h:38 >> + LockedWrapper(DataMutex& dataMutex) > > explicit? Sure. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:703 >> + RefPtr<Type##TrackPrivateGStreamer> track = Type##TrackPrivateGStreamer::create(makeWeakPtr(*this), i, stream); \ > > auto track = I prefer to be explicit, but if more people think it should be auto, okay. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:784 >> + GstStream* stream = gst_stream_collection_get_stream(m_streamCollection.get(), index); > > auto* No. I prefer to be explicit in this case, especially with data types that have refcount implications. I think the extra 5 characters are worth it. >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:57 >> + WebCore::MediaSourceStreamTypeGStreamer streamType() { return m_streamType; } > > isn't this inside WebCore namespace? It is, so yeah, I should be able to remove the `WebCore::`. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:45 >> +using WebCore::SourceBufferPrivateClient; > > why not using namespace WebCore? because of unified builds? Because the WebKitMediaSrc API is not part of the WebCore namespace, but rather the extern "C" namespace. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:159 >> + bool flushing { false }; > > These bools could be packed together with the ones above. Rather than order by data type I prefer to order by semantics, and the flushing flag is more closely related to the condition variables that check for flush than to the initialization event flags. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:166 >> + while (frontIter != queue.end() && !GST_IS_SAMPLE(frontIter->get())) > > The raw loops may be better written with the standard find_if algorithm. > It would be nicer if we had an invariant that everything in the queue is a sample, but I've not read the call-sites to know whether this is possible. We don't have such an invariance. That's why it's Deque<GRefPtr<GstMiniObject>> and not Deque<GRefPtr<GstSample>>. Both samples and events can be enqueued, but for the purposes of calculating durations we need to ignore the latter. That's the only reason I need (very low iteration) loops in the first place. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:197 >> + GRefPtr<GstElement> parentElement = adoptGRef(GST_ELEMENT(gst_element_get_parent(element.get()))); > > I don't believe we need all the refcounting churn in this loop, do we? gst_element_get_parent() return value is transfer full, so we do. At least if we use that API instead of casting to GstObject*, accessing the `parent` field and casting to GstElement*. In any case, it's only used in pipeline dumps and flushes, so it's not a hot codepath by any means. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:259 >> + GRefPtr<GstStreamCollection> newCollection = adoptGRef(gst_stream_collection_new(collection->upstream_id)); > > Why do we need to create new collection and not just append to the end? Because once posted, collections are supposed to be immutable. Quoting gst docs: > Once posted, a GstStreamCollection is immutable. Updates are made by sending a new GstStreamCollection message, which may or may not share some of the GstStream objects from the collection it replaces. The receiver can check the sender of a stream collection message to know which collection is obsoleted. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:263 >> + GRefPtr<GstStream> oldStream = gst_stream_collection_get_stream(collection, i); > > Doubt we need the refcounting gst_stream_collection_add_stream() uses [transfer full] for the stream parameter, so we do. Otherwise the stream would be in two collections while only having a refcount of 1, bad, bad. Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review >>> Source/WebCore/platform/DataMutex.h:36 >>> + class LockedWrapper { >> >> Is it really necessary for the wrapper class to be public? > > Yes, that's how the members of the wrapped object are accessed. Look for examples below. DataMutex::LockedWrapper is a RAII class that holds the mutex and lets you access the data, while DataMutex is the class that stores the data as its contents. OK, I saw it's not owned by LockedWrapper, but missed that it's owned by the DataMutex class. I guess it's just the naming I don't like, then... we can brainstorm other names in a separate WTF review. There's potentially a lot of code that could make use of this for increased safety, so WTF really seems like the right place. And I guess you've noticed all EWS are red. Comment on attachment 374004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374004&action=review >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:703 >>> + RefPtr<Type##TrackPrivateGStreamer> track = Type##TrackPrivateGStreamer::create(makeWeakPtr(*this), i, stream); \ >> >> auto track = > > I prefer to be explicit, but if more people think it should be auto, okay. We normally use it when the type is duplicated, in this case the only thing not explicit would be whether ::create() returns a Ref<>, or RefPtr<>. Feel free to leave it as it is. >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:784 >>> + GstStream* stream = gst_stream_collection_get_stream(m_streamCollection.get(), index); >> >> auto* > > No. I prefer to be explicit in this case, especially with data types that have refcount implications. I think the extra 5 characters are worth it. Similar here when the name of the variable is the name of the type, but again, it was just a suggestion, it's fine either way. >>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:45 >>> +using WebCore::SourceBufferPrivateClient; >> >> why not using namespace WebCore? because of unified builds? > > Because the WebKitMediaSrc API is not part of the WebCore namespace, but rather the extern "C" namespace. I mean using: using namespace WebCore; instead of limiting to those symbols only? Created attachment 374236 [details]
Patch
Attachment 374236 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:37: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:56: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:57: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:58: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:111: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4]
Total errors found: 8 in 27 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 374236 [details] Patch Attachment 374236 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12753326 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374242 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 374236 [details] Patch Attachment 374236 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12753270 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374247 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Comment on attachment 374236 [details] Patch Attachment 374236 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12753391 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374249 [details]
Archive of layout-test-results from ews112 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 374236 [details] Patch Attachment 374236 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12754436 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374267 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 374830 [details]
Patch
Attachment 374830 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:56: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:57: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:58: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:111: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4]
Total errors found: 6 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 374830 [details] Patch Attachment 374830 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12805088 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374839 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 374830 [details] Patch Attachment 374830 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12805315 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374840 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 374830 [details] Patch Attachment 374830 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12805510 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html Created attachment 374848 [details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 374830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374830&action=review I didn't finish yet. Will keep going tomorrow > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2385 > const gchar* playbinName = "playbin"; > > - // MSE doesn't support playbin3. Mediastream requires playbin3. Regular > - // playback can use playbin3 on-demand with the WEBKIT_GST_USE_PLAYBIN3 > - // environment variable. > - if ((!isMediaSource() && g_getenv("WEBKIT_GST_USE_PLAYBIN3")) || url.protocolIs("mediastream")) > + // MSE and Mediastream require playbin3. Regular playback can use playbin3 on-demand with the > + // WEBKIT_GST_USE_PLAYBIN3 environment variable. > + if ((isMediaSource() || url.protocolIs("mediastream") || g_getenv("WEBKIT_GST_USE_PLAYBIN3"))) > playbinName = "playbin3"; I think we should do a GST_INFO with the playbin name we are using just after this. This will save questions in bugzilla about using playbin or playbin3 if we just request the logs. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:315 > + bool samplesHaveDifferentNaturalSize(GstSample* sampleA, GstSample* sampleB) const; doSamplesHaveDifferentNaturalSizes > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:294 > + GRefPtr<GstCaps> caps = appendPipeline->appsinkCaps(); ! > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:83 > + // Only used by URI Handler API implementation . > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:105 > + Stream(WebKitMediaSrc* source, GRefPtr<GstPad>&& pad, AtomString name, WebCore::MediaSourceStreamTypeGStreamer type, > + GRefPtr<GstCaps>&& initialCaps, GRefPtr<GstStream>&& streamInfo) You can make this a single line. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:112 > + , streamingMembersDataMutex(WTFMove(initialCaps), source->priv->startTime, source->priv->rate, > + adoptGRef(gst_event_new_stream_collection(source->priv->collection.get()))) style > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:150 > + bool pushedFirstBuffer { false }; > + bool streamStartSent { false }; > + bool needsSegmentEvent { true }; hasPushedFirstBuffer wasStreamStartSent doesNeedSegmentEvent > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:160 > + Condition padLinkedOrFlushed; > + Condition queueChangedOrFlushed; > + Deque<GRefPtr<GstMiniObject>> queue; > + bool flushing { false }; > + bool needsToNotifyOnLowWaterLevel { false }; padLinkedOrFlushedCondition queueChangedOrFlushedCondition isFlushing doesNeedToNotifyOnLowWaterLevel > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:162 > + guint64 durationEnqueued() const uint64_t > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:164 > + // Find the first and last GstSample in the queue and subtract their DTS . > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:187 > + bool removed { false }; wasRemoved > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:216 > + gst_element_class_set_static_metadata(eklass, "WebKit MediaSource source element", > + "Source", "Feeds samples coming from WebKit MediaSource object", "Igalia <aboya@igalia.com>"); Single line > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:250 > +GRefPtr<GstStreamCollection> collectionPlusStream(GstStreamCollection* collection, GRefPtr<GstStream> stream) addStreamToCollection and I think it would make sense to get a && for the stream > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:255 > + guint n = gst_stream_collection_get_size(collection); > + for (guint i = 0; i < n; i++) { unsigned, unsigned > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:257 > + GRefPtr<GstStream> oldStream = gst_stream_collection_get_stream(collection, i); > + gst_stream_collection_add_stream(newCollection.get(), oldStream.leakRef()); gst_stream_collection_add_stream(newCollection.get(), gst_object_ref(gst_stream_collection_get_stream(collection, i))); And you don't need the { } > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:264 > +GRefPtr<GstStreamCollection> collectionMinusStream(GstStreamCollection* collection, const GstStream* stream) removeStreamFromCollection > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:269 > + guint n = gst_stream_collection_get_size(collection); > + for (guint i = 0; i < n; i++) { unsigned, unsigned > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:294 > +void webKitMediaSrcAddStream(WebKitMediaSrc* source, AtomString name, WebCore::MediaSourceStreamTypeGStreamer type, > + GRefPtr<GstCaps>&& initialCaps) single line > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:300 > + GRefPtr<GstStream> streamInfo = adoptGRef(gst_stream_new(name.string().utf8().data(), initialCaps.get(), > + gstStreamType(type), GST_STREAM_FLAG_SELECT)); single line > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:339 > + // Flush the source element **and** downstream. We want to stop the streaming thread and for that we need all > + // elements downstream to be idle. single line Comment on attachment 374830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374830&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2385 >> playbinName = "playbin3"; > > I think we should do a GST_INFO with the playbin name we are using just after this. This will save questions in bugzilla about using playbin or playbin3 if we just request the logs. There's one already a few lines below: GST_INFO_OBJECT(pipeline(), "Using legacy playbin element: %s", boolForPrinting(m_isLegacyPlaybin)); >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:216 >> + "Source", "Feeds samples coming from WebKit MediaSource object", "Igalia <aboya@igalia.com>"); > > Single line Source/Network Comment on attachment 374830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374830&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:112 >> + adoptGRef(gst_event_new_stream_collection(source->priv->collection.get()))) > > style What is the correct style here? Putting it in one line so that the style checker does not fail? >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:250 >> +GRefPtr<GstStreamCollection> collectionPlusStream(GstStreamCollection* collection, GRefPtr<GstStream> stream) > > addStreamToCollection and I think it would make sense to get a && for the stream No, because it returns a does not add a stream to a collection. It creates a new collection that is the sum of the previous collection with the new item, therefore the name. Since this seems to be tripping a lot of people, I will add a comment explaining it: // GstStreamCollection are immutable objects one posted. THEY MUST NOT BE MODIFIED once they have been posted. // Instead, when stream changes occur a new collection must be made. The following functions help to create // such new collections: // (methods) Adding a && for the stream here is not helpful since in the only use case the variable is necessarily copied, not moved (it still has to be used later). Also, since a refcount increase is necessary, at gst_stream_collection_add_stream() at the latest, there is no performance penalty on doing it this way. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:264 >> +GRefPtr<GstStreamCollection> collectionMinusStream(GstStreamCollection* collection, const GstStream* stream) > > removeStreamFromCollection Same as before. (It does not remove the stream from the collection, so the current name is better.) Created attachment 374897 [details]
Patch
Attachment 374897 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:56: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:57: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:58: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
Total errors found: 5 in 29 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Alicia Boya García from comment #33) > Comment on attachment 374830 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374830&action=review > > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:112 > >> + adoptGRef(gst_event_new_stream_collection(source->priv->collection.get()))) > > > > style > > What is the correct style here? Putting it in one line so that the style > checker does not fail? The comma you fixed already. > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:250 > >> +GRefPtr<GstStreamCollection> collectionPlusStream(GstStreamCollection* collection, GRefPtr<GstStream> stream) > > > > addStreamToCollection and I think it would make sense to get a && for the stream > > No, because it returns a does not add a stream to a collection. It creates a > new collection that is the sum of the previous collection with the new item, > therefore the name. Since this seems to be tripping a lot of people, I will > add a comment explaining it: > // GstStreamCollection are immutable objects one posted. THEY MUST NOT BE > MODIFIED once they have been posted. > // Instead, when stream changes occur a new collection must be made. The > following functions help to create > // such new collections: > > // (methods) You're right with the reasoning but your proposal is still not good according to the style and good practices of WebKit. > Adding a && for the stream here is not helpful since in the only use case > the variable is necessarily copied, not moved (it still has to be used > later). Also, since a refcount increase is necessary, at > gst_stream_collection_add_stream() at the latest, there is no performance > penalty on doing it this way. I understand the whole case and I still don't agree with your assessment because even when your reference counting is ok, there's still a matter of interface definition. Your are not moving the variable because you are using it after calling that function but from the point of view of the interface of that function it is not something to consider. That function needs to own that reference and it does not care how you do it and what you do later with that object. It just needs to own a reference. With && you can do that very efficiently. If you didn't use the variable after calling that function it would be a good idea to pass &&, right? So forget about the use from the POV of this function interface and get a &&. Now from the POV of the caller: shite, this functions signature is asking for a && and I need the object for later. No problem, let's pass a copy to it by invoking its constructor and since it is already and r-value, no need for moving. Considering the current code, what you wrote works, but if case you didn't need to use the variable later, moving would be more efficient. > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:264 > >> +GRefPtr<GstStreamCollection> collectionMinusStream(GstStreamCollection* collection, const GstStream* stream) > > > > removeStreamFromCollection > > Same as before. (It does not remove the stream from the collection, so the > current name is better.) (In reply to Xabier Rodríguez Calvar from comment #36) > > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:250 > > >> +GRefPtr<GstStreamCollection> collectionPlusStream(GstStreamCollection* collection, GRefPtr<GstStream> stream) > > > > > > addStreamToCollection and I think it would make sense to get a && for the stream > > > > No, because it returns a does not add a stream to a collection. It creates a > > new collection that is the sum of the previous collection with the new item, > > therefore the name. Since this seems to be tripping a lot of people, I will > > add a comment explaining it: > > > // GstStreamCollection are immutable objects one posted. THEY MUST NOT BE > > MODIFIED once they have been posted. > > // Instead, when stream changes occur a new collection must be made. The > > following functions help to create > > // such new collections: > > > > // (methods) > > You're right with the reasoning but your proposal is still not good > according to the style and good practices of WebKit. (I hadn't finished commenting this, just wrote something below and fast-fingered the submit comment button) I know sometimes it is not easy, but I think you can spend a couple of minutes and find something better according to the style. Btw, same goes for the remove one. (In reply to Philippe Normand from comment #32) > >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2385 > >> playbinName = "playbin3"; > > > > I think we should do a GST_INFO with the playbin name we are using just after this. This will save questions in bugzilla about using playbin or playbin3 if we just request the logs. > > There's one already a few lines below: > GST_INFO_OBJECT(pipeline(), "Using legacy playbin element: %s", > boolForPrinting(m_isLegacyPlaybin)); Might be, but I tried to open the code below this and bugzilla says this does not apply. Neither does the new patch, btw. Anyway, if there's something already, good. Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:499 > + if (ret != GST_FLOW_OK) > + GST_WARNING_OBJECT(pad, "Pushing buffer returned %s", gst_flow_get_name(ret)); Shouldn't the element emit an error here if ret is not GST_FLOW_OK ? Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review We're close to finishing, I think. Anyway, it would be interesting if Phil and Enrique could have a look at this as well, because they know the flows better and I do. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:772 > + m_videoSize = FloatSize(); // Force re-calculation in next call to naturalSize() . > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:315 > + bool samplesHaveDifferentNaturalSize(GstSample* sampleA, GstSample* sampleB) const; doSamplesHaveDifferentNaturalSizes > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:750 > + // As it is now, resetParserState() will cause the pads to be disconnected, so they will later be re-added on the > + // next initialization segment. Personal nit: single line > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:55 > + GRefPtr<GstCaps>& appsinkCaps() { return m_appsinkCaps; } Either const this of remove the &. I don't think it is a good idea to provide just plain unprotected references. If I can device, I'd go for the const. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:62 > + void seekCompleted(); reportSeekCompleted > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:106 > + bool m_pipelinePlaying = true; m_isPipelinePlaying > Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:128 > + bool readyForMoreSamples = m_client->isReadyForMoreSamples(trackId); isReadyForMoreSamples > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:69 > + RefPtr<Stream> streamByName(AtomString name) > + { > + RefPtr<Stream> stream = streams.get(name); > + ASSERT(stream); > + return stream; I'd take a const AtomString& and return a const RefPtr&<Stream> if possible to avoid unnecessary refence increases. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:84 > + GUniquePtr<gchar> uri; char > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:187 > + bool readyForMoreSamples { true }; isReadyForMoreSamples > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:247 > +// GstStreamCollection are immutable objects one posted. THEY MUST NOT BE MODIFIED once they have been posted. one -> once > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:251 > +GRefPtr<GstStreamCollection> collectionPlusStream(GstStreamCollection* collection, GRefPtr<GstStream> stream) name. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:337 > + webKitMediaSrcStreamFlushStop(stream, FALSE); false > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:349 > + GST_ERROR("Unexpected pad mode in WebKitMediaSrc"); GST_ERROR_OBJECT? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:383 > +static void webKitMediaSrcStreamNotifyLowWaterLevel(RefPtr<Stream> stream) I think here you can take a RefPtr<Stream>& because the way you capture it in the lambda closure is going to copy it. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:400 > +// called with STREAM_LOCK // Called with STREAM_LOCK. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:436 > + GUniquePtr<gchar> streamId(g_strdup_printf("mse/%s", stream->name.string().utf8().data())); char > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:437 > + GRefPtr<GstEvent> event = adoptGRef(gst_event_new_stream_start(streamId.get())); Considering that you're creating the object here ans just pushing it three lines below, I wouln't even adopt it into a smart ptr. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:446 > + GRefPtr<GstEvent> event = adoptGRef(gst_event_new_caps(streamingMembers->pendingInitialCaps.get())); ditto > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:487 > + GUniquePtr<gchar> fileName { g_strdup_printf("play-first-buffer-%s", stream->name.string().utf8().data()) }; char > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:505 > + GRefPtr<GstEvent> event = GRefPtr<GstEvent>(GST_EVENT(object.leakRef())); > + > + streamingMembers.lockHolder().unlockEarly(); > + bool eventHandled = gst_pad_push_event(pad, GRefPtr<GstEvent>(event).leakRef()); I don't think we need such convolution with the references here. I'd better do: GstEvent* event = GST_EVENT(object.get()): ... ... gst_pad_push_event(pad, gst_event_ref(event)); I don't have a strong opinion on this though. For me it looks quite convoluted and it makes clear that GRefPtr could have a copyRef method. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:512 > +static void webKitMediaSrcEnqueueObject(WebKitMediaSrc* source, AtomString streamName, GRefPtr<GstMiniObject>&& object) const AtomString& here and up the onion? (EnqueueSample, Event, EOS, etc.) > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:568 > +static void webKitMediaSrcStreamFlushStart(RefPtr<Stream>& stream) is it possible to const RefPtr<Stream>& and the stop? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:627 > + gst_element_query_position(findPipeline(GRefPtr<GstElement>(GST_ELEMENT(source))).get(), GST_FORMAT_TIME, > + reinterpret_cast<gint64*>(&pipelineStreamTime)); Single line? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:629 > + // -1 is returned when the pipeline is not yet pre-rolled (e.g. just after a seek). In this case we don't need to > + // adjust the segment though, as running time has not advanced. ditto? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:652 > + RefPtr<Stream> stream = pair.value; Why do you need to increase the reference here? Can't you just auto& ? > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:681 > + // Barring pipeline dumps someone may add during debugging, WebKit will only read these properties > + // (n-video etc.) from the main thread. single line > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.h:67 > +void webKitMediaSrcAddStream(WebKitMediaSrc*, AtomString name, WebCore::MediaSourceStreamTypeGStreamer, > + GRefPtr<GstCaps>&& initialCaps); single line? I couldn't make this work with GStreamer 1.14.4. The minimum version required should probably now be bumped to 1.16.0 :) Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-653 > - if (!g_strcmp0(mediaType, "audio/x-vorbis")) { > - GstElement* vorbisparse = gst_element_factory_make("vorbisparse", parserName.get()); > - ASSERT(vorbisparse); > - g_return_val_if_fail(vorbisparse, nullptr); > - return GRefPtr<GstElement>(vorbisparse); > - } Why is this removed? Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:-253 > -static void webkit_media_src_init(WebKitMediaSrc* source) This is gone now but it's still needed, at least for this: GST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE); > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:301 > + GRefPtr<GstPad> pad = gst_pad_new_from_static_template(&srcTemplate, name.string().utf8().data()); This sets the pad name to things like A1 or V1, which doesn't correspond with the pad template (src_%u). In those cases I think there should be 2 pad templates, audio_%u and video_%u, and the names should be audio_1, video_1, etc. :) > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:491 > + if (!streamingMembers->hasPushedFirstBuffer) { > + GUniquePtr<gchar> fileName { g_strdup_printf("play-first-buffer-%s", stream->name.string().utf8().data()) }; > + GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(findPipeline(GRefPtr<GstElement>(GST_ELEMENT(stream->source))).get()), > + GST_DEBUG_GRAPH_SHOW_ALL, fileName.get()); > + streamingMembers->hasPushedFirstBuffer = true; > } How useful is this actually? The dumps I could see here were showing incomplete pipelines. Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review Pretty nice patch :-) > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:362 > + // Position queries on a null pipeline return 0. If we're at on a not running pipeline? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:363 > + // the end of the stream the pipeline is null but we want to on a pipeline in NULL state? (this sounds like the m_pipeline == nullptr) > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:740 > + GST_DEBUG_OBJECT(pipeline(), "Inspecting stream collection: %s", gst_stream_collection_get_upstream_id(m_streamCollection.get())); `GST_DEBUG_OBJECT(pipeline(), "Inspecting stream collection: %" GST_PTR_FORMAT, m_streamCollection.get()); ` ? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:778 > + m_notifier->notify(MainThreadNotification::SizeChanged, [this, newSize = naturalSizeFromCaps(gst_sample_get_caps(sample))] { `newSize` isn't used in the callback? > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:9 > + * Copyright (C) 2019 Igalia S.L. "2019" should be appended to line 6 > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:151 > + webKitMediaSrcSeek(WEBKIT_MEDIA_SRC(m_source.get()), toGstClockTime(m_seekTime), m_playbackRate); Why can't the webkitMediaSrc handle seek event the usual way? > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:226 > +void MediaPlayerPrivateGStreamerMSE::didEnd() Shouldn't that linkup and just call `invalidateCachedPosition()`? > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:305 > + webKitMediaSrcAddStream(WEBKIT_MEDIA_SRC(m_source.get()), newTrack->id(), appendPipeline->streamType(), WTFMove(caps)); You should probably make `m_source` a WebkitMediaSrc as you cast it everywhere you use it right now > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:77 > + GRefPtr<GstStreamCollection> collection { adoptGRef(gst_stream_collection_new("WebKitMediaSrc")) }; Stream IDs are supposed to be unique (same goes for upstream_stream_id I believe) > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:156 > + Deque<GRefPtr<GstMiniObject>> queue; Maybe use a `GstDataQueue` instead? It has been designed exactly for that. > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:242 > + Stream* stream = static_cast<Stream*>(g_object_get_data(G_OBJECT(pad), "webkit-stream")); Would probably be better to subclass GstPad for your pads. Actually all the `struct Stream` could be in the pad subclass, this would better match what we recommand in GStreamer. Using object data is not recommended and generally not thread safe. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:499 >> + GST_WARNING_OBJECT(pad, "Pushing buffer returned %s", gst_flow_get_name(ret)); > > Shouldn't the element emit an error here if ret is not GST_FLOW_OK ? I think you should use a `GstFlowCombiner` as any elements with several source pad (In reply to Xabier Rodríguez Calvar from comment #36) > You're right with the reasoning but your proposal is still not good > according to the style and good practices of WebKit. Can you please point the rule that the current name (collectionPlusStream()) is contradicting so I can find a better one? Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:363 >> + // the end of the stream the pipeline is null but we want to > > on a pipeline in NULL state? (this sounds like the m_pipeline == nullptr) You got a point. I'm rewriting the full block for more clarity. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:740 >> + GST_DEBUG_OBJECT(pipeline(), "Inspecting stream collection: %s", gst_stream_collection_get_upstream_id(m_streamCollection.get())); > > `GST_DEBUG_OBJECT(pipeline(), "Inspecting stream collection: %" GST_PTR_FORMAT, m_streamCollection.get()); ` ? Actually, the GST_PTR_FORMATTER of GstStreamCollection (gst_info_describe_stream_collection()) doesn't print the upstream-id, which is an important bit of information (it's different when it procedes from WebKitMediaSrc than when it has been made by parsebin or decodebin3). I could implement a mixed solution, printing both. >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-653 >> - } > > Why is this removed? Unlike other parsers, vorbisparse has latency (in the sense that when it gets a chain call with a series of complete frames, it may not emit the parsed frames until another chain in the future), which makes it inappropriate for AppendPipeline, as there is no good way I know to flush it. But actually vorbisparse is not known to be necessary and it was only introduced for consistency with other formats. Parsers are used in AppendPipeline to reconstruct information that is lost due to poor muxes. There have been no reported cases of this being a problem with Vorbis in WebM, so I'm just removing the parser. I found about this issue when debugging some WPT MSE tests that were failing for Vorbis WebM files. It could very well go in a different patch. >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:151 >> + webKitMediaSrcSeek(WEBKIT_MEDIA_SRC(m_source.get()), toGstClockTime(m_seekTime), m_playbackRate); > > Why can't the webkitMediaSrc handle seek event the usual way? Seeks, it technically could but that's not the design of the new WebKitMediaSrc. WebKitMediaSrc is designed to be controlled with its own C++ API, defined in WebKitMediaSourceGStreamer.h and that alone. For consistency, seeks are implemented this way. There is no value in adding indirection layers by sending and parsing GStreamer events here, so simple wins. >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:226 >> +void MediaPlayerPrivateGStreamerMSE::didEnd() > > Shouldn't that linkup and just call `invalidateCachedPosition()`? Not necessarily. For instance, MediaPlayerPrivateGStreamer::didEnd() calls durationChanged() but we don't want nor need that for MSE, at least not here. >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:305 >> + webKitMediaSrcAddStream(WEBKIT_MEDIA_SRC(m_source.get()), newTrack->id(), appendPipeline->streamType(), WTFMove(caps)); > > You should probably make `m_source` a WebkitMediaSrc as you cast it everywhere you use it right now I would love to, but m_source is defined in a parent class, so I can't (in the non-MSE case it's a different element). >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:62 >> + void seekCompleted(); > > reportSeekCompleted The name comes from MediaSourcePrivate. Do we want to deviate from it? >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:77 >> + GRefPtr<GstStreamCollection> collection { adoptGRef(gst_stream_collection_new("WebKitMediaSrc")) }; > > Stream IDs are supposed to be unique (same goes for upstream_stream_id I believe) AFAIK it only matters that they are unique within the pipeline. decodebin3 also uses a static string for upstream-id. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:156 >> + Deque<GRefPtr<GstMiniObject>> queue; > > Maybe use a `GstDataQueue` instead? It has been designed exactly for that. I didn't know about it, but certainly it looks like it from a quick survey. That would be a big change at this point though, and I would need to check that it actually has everything I need and no more complexity than the current implementation. For starters, it's locked where as my current queue uses the Stream lock already for that (which I need for other reasons, and there would be no reason to split it), so that would be an unnecessary mutex. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:242 >> + Stream* stream = static_cast<Stream*>(g_object_get_data(G_OBJECT(pad), "webkit-stream")); > > Would probably be better to subclass GstPad for your pads. > > Actually all the `struct Stream` could be in the pad subclass, this would better match what we recommand in GStreamer. Using object data is not recommended and generally not thread safe. That seems a bit overkill to me. Neither qtdemux nor matroskademux do that. Do you have any example? >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:-253 >> -static void webkit_media_src_init(WebKitMediaSrc* source) > > This is gone now but it's still needed, at least for this: GST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE); So... how would I reconciliate that with using WEBKIT_DEFINE_TYPE_WITH_CODE(), which creates its own non-overridable implementation of the _init() method? (It only allows to add code to the _get_type_once() method). >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:301 >> + GRefPtr<GstPad> pad = gst_pad_new_from_static_template(&srcTemplate, name.string().utf8().data()); > > This sets the pad name to things like A1 or V1, which doesn't correspond with the pad template (src_%u). In those cases I think there should be 2 pad templates, audio_%u and video_%u, and the names should be audio_1, video_1, etc. :) Good catch. I will change the template to src_%s and use makeString("src_", name). I disagree on making separate pad templates for audio and video though. As far as the operation of this element is concerned, video and audio work the same and I would need a strong reason to create different kinds of pads for each. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:437 >> + GRefPtr<GstEvent> event = adoptGRef(gst_event_new_stream_start(streamId.get())); > > Considering that you're creating the object here ans just pushing it three lines below, I wouln't even adopt it into a smart ptr. I explained my stance on this in another comment. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:491 >> } > > How useful is this actually? The dumps I could see here were showing incomplete pipelines. When I am debugging pad-not-linked errors on the first push (think playbin3 not linking the source element) I want to see the state of the pipeline when that occurred. The file name could be more explanatory though. What about "play-before-first-push"? >>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:499 >>> + GST_WARNING_OBJECT(pad, "Pushing buffer returned %s", gst_flow_get_name(ret)); >> >> Shouldn't the element emit an error here if ret is not GST_FLOW_OK ? > > I think you should use a `GstFlowCombiner` as any elements with several source pad WebKitMediaSrc is a source element. It does not return anything because it has no upstream. The function returns void. For the same reason GstFlowCombiner does not apply in this case (and I'm extra happy it doesn't because otherwise that would the first and only thing that would need locking with a granularity of the whole element in the entire loop function.) >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:505 >> + bool eventHandled = gst_pad_push_event(pad, GRefPtr<GstEvent>(event).leakRef()); > > I don't think we need such convolution with the references here. I'd better do: > GstEvent* event = GST_EVENT(object.get()): > ... > ... gst_pad_push_event(pad, gst_event_ref(event)); > > I don't have a strong opinion on this though. For me it looks quite convoluted and it makes clear that GRefPtr could have a copyRef method. I took a radical approach to GRefPtr<> so that it's obvious when transfer-none or transfer-full is involved, since there are lots of places where that is not obvious. Refcount errors due to using the wrong transfer when using GRefPtr<> are caught in the same line the transfer is made. When using C-style pointers this is much less obvious. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:512 >> +static void webKitMediaSrcEnqueueObject(WebKitMediaSrc* source, AtomString streamName, GRefPtr<GstMiniObject>&& object) > > const AtomString& here and up the onion? (EnqueueSample, Event, EOS, etc.) I assumed AtomStrings were cheap, but enough places use const& that I'm convinced now to do the same. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:568 >> +static void webKitMediaSrcStreamFlushStart(RefPtr<Stream>& stream) > > is it possible to const RefPtr<Stream>& and the stop? It's possible in most places, as long as the survival of the Stream is guaranteed for the lifetime of the variable. The exception to that would be when it's passed as a lambda capture for a main thread task, as it's needed for memory safety that the Stream still exists by the time the main loop runs the task. >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:627 >> + reinterpret_cast<gint64*>(&pipelineStreamTime)); > > Single line? Honestly, I like being able to read code in my laptop at a comfortable size without scrolling horizontally. When the code out of screen is something obvious or not very important, like variables being printed in a debug print call, I'm OK with having long lines since they take less space when reading the actual important code... but in other cases where it actually matters I prefer one more line that will for sure be read than "invisible code" that would be skipped. My gut tells me to keep it this way here. Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review >>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:242 >>> + Stream* stream = static_cast<Stream*>(g_object_get_data(G_OBJECT(pad), "webkit-stream")); >> >> Would probably be better to subclass GstPad for your pads. >> >> Actually all the `struct Stream` could be in the pad subclass, this would better match what we recommand in GStreamer. Using object data is not recommended and generally not thread safe. > > That seems a bit overkill to me. Neither qtdemux nor matroskademux do that. Do you have any example? https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/webrtc/gstwebrtcbin.c#L182 >>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:301 >>> + GRefPtr<GstPad> pad = gst_pad_new_from_static_template(&srcTemplate, name.string().utf8().data()); >> >> This sets the pad name to things like A1 or V1, which doesn't correspond with the pad template (src_%u). In those cases I think there should be 2 pad templates, audio_%u and video_%u, and the names should be audio_1, video_1, etc. :) > > Good catch. I will change the template to src_%s and use makeString("src_", name). > > I disagree on making separate pad templates for audio and video though. As far as the operation of this element is concerned, video and audio work the same and I would need a strong reason to create different kinds of pads for each. I don't have strong reasons to give. I think it's a nice convention to adopt, as in the mediastreamsrc element, but if you don't want, so be it :) >>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:491 >>> } >> >> How useful is this actually? The dumps I could see here were showing incomplete pipelines. > > When I am debugging pad-not-linked errors on the first push (think playbin3 not linking the source element) I want to see the state of the pipeline when that occurred. The file name could be more explanatory though. What about "play-before-first-push"? Then I'd suggest to move this below, when you know the result of gst_pad_push() which can be a pad-not-linked error. >>>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:499 >>>> + GST_WARNING_OBJECT(pad, "Pushing buffer returned %s", gst_flow_get_name(ret)); >>> >>> Shouldn't the element emit an error here if ret is not GST_FLOW_OK ? >> >> I think you should use a `GstFlowCombiner` as any elements with several source pad > > WebKitMediaSrc is a source element. It does not return anything because it has no upstream. The function returns void. > > For the same reason GstFlowCombiner does not apply in this case (and I'm extra happy it doesn't because otherwise that would the first and only thing that would need locking with a granularity of the whole element in the entire loop function.) Sure, it returns void, but you can still pause the task and emit an error message anyway :) (In reply to Alicia Boya García from comment #45) > (In reply to Xabier Rodríguez Calvar from comment #36) > > You're right with the reasoning but your proposal is still not good > > according to the style and good practices of WebKit. > > Can you please point the rule that the current name (collectionPlusStream()) > is contradicting so I can find a better one? # Use descriptive verbs in function names. (In reply to Alicia Boya García from comment #46) > > >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:62 > >> + void seekCompleted(); > > > > reportSeekCompleted > > The name comes from MediaSourcePrivate. Do we want to deviate from it? If we are overriding the function, then let's mark it as override and keep the name. If we are not overriding the function, then let's write a proper name. > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:437 > >> + GRefPtr<GstEvent> event = adoptGRef(gst_event_new_stream_start(streamId.get())); > > > > Considering that you're creating the object here ans just pushing it three lines below, I wouln't even adopt it into a smart ptr. > > I explained my stance on this in another comment. Which one? > > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:568 > >> +static void webKitMediaSrcStreamFlushStart(RefPtr<Stream>& stream) > > > > is it possible to const RefPtr<Stream>& and the stop? > > It's possible in most places, as long as the survival of the Stream is > guaranteed for the lifetime of the variable. The exception to that would be > when it's passed as a lambda capture for a main thread task, as it's needed > for memory safety that the Stream still exists by the time the main loop > runs the task. When you capture in a lambda, copy constructor is used (unless specified otherwise), so reference is increased. > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:627 > >> + reinterpret_cast<gint64*>(&pipelineStreamTime)); > > > > Single line? > > Honestly, I like being able to read code in my laptop at a comfortable size > without scrolling horizontally. When the code out of screen is something > obvious or not very important, like variables being printed in a debug print > call, I'm OK with having long lines since they take less space when reading > the actual important code... but in other cases where it actually matters I > prefer one more line that will for sure be read than "invisible code" that > would be skipped. > > My gut tells me to keep it this way here. I share your rationale but it looks like your font is much bigger than mine. Whenever I told you to use a single line was because your could fit in a single line and there was still space left. So I was even being conservative most of the times. Comment on attachment 374897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374897&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:69 >> + return stream; > > I'd take a const AtomString& and return a const RefPtr&<Stream> if possible to avoid unnecessary refence increases. Taking a const AtomString& is OK, but returning a const RefPtr<Stream>& is not. I actually made that change and kept going until later I found many strange crashes. Problem is, even if the value type on the HashMap is RefPtr<Stream>, streams.get() actually returns a Stream* ("peek type"), and is the assignation to a variable that creates the RefPtr<>. So if you assign it to const RefPtr<Stream>& and return it, you are returning the address of a temporary and memory corruption occurs. I can return Stream* though... And any caller that needs a ref-count increase can assign the Stream* to a RefPtr<Stream> variable just like this function was doing before. (In reply to Alicia Boya García from comment #50) > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:69 > >> + return stream; > > > > I'd take a const AtomString& and return a const RefPtr&<Stream> if possible to avoid unnecessary refence increases. > > Taking a const AtomString& is OK, but returning a const RefPtr<Stream>& is > not. I actually made that change and kept going until later I found many > strange crashes. > > Problem is, even if the value type on the HashMap is RefPtr<Stream>, > streams.get() actually returns a Stream* ("peek type"), and is the > assignation to a variable that creates the RefPtr<>. So if you assign it to > const RefPtr<Stream>& and return it, you are returning the address of a > temporary and memory corruption occurs. > > I can return Stream* though... And any caller that needs a ref-count > increase can assign the Stream* to a RefPtr<Stream> variable just like this > function was doing before. I hadn't taken into account copy elision and was not taking into account either the cambalache of HashMap type handling. You can keep it as it is then. (In reply to Xabier Rodríguez Calvar from comment #51) > (In reply to Alicia Boya García from comment #50) > > >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:69 > > >> + return stream; > > > > > > I'd take a const AtomString& and return a const RefPtr&<Stream> if possible to avoid unnecessary refence increases. > > > > Taking a const AtomString& is OK, but returning a const RefPtr<Stream>& is > > not. I actually made that change and kept going until later I found many > > strange crashes. > > > > Problem is, even if the value type on the HashMap is RefPtr<Stream>, > > streams.get() actually returns a Stream* ("peek type"), and is the > > assignation to a variable that creates the RefPtr<>. So if you assign it to > > const RefPtr<Stream>& and return it, you are returning the address of a > > temporary and memory corruption occurs. > > > > I can return Stream* though... And any caller that needs a ref-count > > increase can assign the Stream* to a RefPtr<Stream> variable just like this > > function was doing before. > > I hadn't taken into account copy elision and was not taking into account > either the cambalache of HashMap type handling. You can keep it as it is > then. Another option would be using find() instead of get(). You'd ASSERT no the returned iterator not being .end() and then you can return the const RefPtr& from the iterator. If you handle that const RefPtr& then we'll save the ref increase/ decrease. (In reply to Philippe Normand from comment #47) > Sure, it returns void, but you can still pause the task and emit an error > message anyway :) I can do this for gst_pad_push() but not for gst_pad_push(), since the latter only return true/false, but false is also returned in the case of a flush and that is not an error condition. (In reply to Philippe Normand from comment #47) > Then I'd suggest to move this below, when you know the result of > gst_pad_push() which can be a pad-not-linked error. I was just giving an example. In general, when debugging this in the future I may want to know what the pipeline with the source element looked before playback... or I may need something else, but this is a need I had in the past several times and I'm leaving it there for potential future debugging sessions. If someone objects to it, I could remove it completely and rewrite it next time I think it could be useful, but overall I like keeping pipeline dumps handy, as they have proven very useful many times in the past. (In reply to Thibault Saunier from comment #44) > > + Deque<GRefPtr<GstMiniObject>> queue; > > Maybe use a `GstDataQueue` instead? It has been designed exactly for that. I looked at it, read the doc and a bit of the code, even started writing an experimental patch to see how it would actually look but after that I decided to keep it as is. Although GstDataQueue looks useful and it seems to implements the functionality I could need from a queue, it's still a net increase of complexity in this case. Particularly problematic is the fact that it has its own mutex, flushing flag and condition variable, separate from the mutex, flushing flag and condition variable I already have and still need, yet private to the queue. Using both at the same time gets confusing quickly, and unnecessarily so. In the end, any small gain in code brevity is lost to additional complexity, which is not a trade off worth it. Created attachment 376041 [details]
Patch
Created attachment 376044 [details]
Patch
Attachment 376044 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:56: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:57: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:58: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:135: webkit_media_src_pad_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:140: webkit_media_src_pad_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:277: webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 8 in 31 files
If any of these errors are false positives, please file a bug against check-webkit-style.
This looks very good to me! Regarding style bot, you can skip the style-check for the Gstreamer element impl. Grep gst_ in Tools/Scripts/webkitpy and you should find where the file can be white-listed. Created attachment 376688 [details]
Patch
Created attachment 376690 [details]
Patch
Comment on attachment 376690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376690&action=review > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:375 > + // Workaround: gst_element_add_pad() should already call gst_pad_set_active() if the element is PAUSED or > + // PLAYING. Unfortunately, as of GStreamer 1.14.4 it does so with the element lock taken, causing a deadlock > + // in gst_pad_start_task(), who tries to post a `stream-status` message in the element, which also requires > + // the element lock. Activating the pad beforehand avoids that codepath. > + GstState state; > + gst_element_get_state(GST_ELEMENT(source), &state, nullptr, 0); > + if (state > GST_STATE_READY) > + gst_pad_set_active(GST_PAD(pad.get()), true); Would it make sense to have a runtime GStreamer version check wrapping this code? Comment on attachment 376690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376690&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:375 >> + gst_pad_set_active(GST_PAD(pad.get()), true); > > Would it make sense to have a runtime GStreamer version check wrapping this code? There is no released version of GStreamer that performs automatic pad activation without deadlock yet. There is a merge request pending review in GStreamer: https://gitlab.freedesktop.org/gstreamer/gstreamer/merge_requests/210#note_194529 Since performing the activation manually will always work, and GStreamer versions without the fix are going to stay for quite a while, I think it's better to have all users run the same code path. Take the comment as informative of why I need to do that. Comment on attachment 376690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376690&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:750 > + bool firstTimeConnectingTrack = (m_track == nullptr); These () are not needed. and isFirstTimeConn... > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:8 > + * Copyright (C) 2009, 2010, 2011, 2012, 2013, 2016, 2017, 2019 Igalia S.L > * Copyright (C) 2015 Sebastian Dröge <sebastian@centricular.com> > - * Copyright (C) 2015, 2016, 2017 Metrological Group B.V. > + * Copyright (C) 2015, 2016, 2017, 2019 Metrological Group B.V. I think you can also add 2018, AFAIK, we didn't stop working here. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:213 > + if (!changePipelineState(GST_STATE_PLAYING)) { Remove the { } > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:218 > + if (!changePipelineState(GST_STATE_PAUSED)) { ditto > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:7 > + * Copyright (C) 2015, 2016, 2019 Metrological Group B.V. > + * Copyright (C) 2015, 2016, 2019 Igalia, S.L 2018 > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:409 > + // Unblock the streaming thread . > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:526 > + // This sample needs new caps (typically because of a quality change) . Created attachment 377446 [details]
Patch
Comment on attachment 377446 [details] Patch Clearing flags on attachment: 377446 Committed r249205: <https://trac.webkit.org/changeset/249205> All reviewed patches have been landed. Closing bug. Note the GStreamer 1.16 requirement means Ubuntu, Debian, etc. will all need to disable MSE in order to upgrade to 2.26. |