Bug 162902 - [GStreamer][MSE] WebKitMediaSourceGStreamer refactoring
Summary: [GStreamer][MSE] WebKitMediaSourceGStreamer refactoring
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords:
Depends on:
Blocks: 157314
  Show dependency treegraph
 
Reported: 2016-10-04 06:11 PDT by Enrique Ocaña
Modified: 2016-10-26 01:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (58.09 KB, patch)
2016-10-04 06:16 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (57.76 KB, patch)
2016-10-16 12:07 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (57.63 KB, patch)
2016-10-21 14:28 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (57.64 KB, patch)
2016-10-21 14:41 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (57.62 KB, patch)
2016-10-25 02:24 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.24 MB, application/zip)
2016-10-25 03:33 PDT, Build Bot
no flags Details
Patch (57.62 KB, patch)
2016-10-26 01:21 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2016-10-04 06:11:59 PDT
Move WebKitMediaSourceGStreamer to the mse directory, split public and private header sections, manage stream data throttling and seek synchronization, audio/video/text stream counting, improve GStreamer duration query, stream initialization, remove MediaSourceClientGStreamer logic (factored out to its own class in another patch) and interaction with MediaPlayerPrivateGStreamerMSE, PlaybackPipeline and SourceBufferPrivateGStreamer.
Comment 1 Enrique Ocaña 2016-10-04 06:16:21 PDT
Created attachment 290598 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-04 06:17:48 PDT
Attachment 290598 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:78:  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/WebKitMediaSourceGStreamerPrivate.h:79:  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/WebKitMediaSourceGStreamerPrivate.h:80:  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/WebKitMediaSourceGStreamerPrivate.h:81:  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/WebKitMediaSourceGStreamerPrivate.h:82:  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/WebKitMediaSourceGStreamerPrivate.h:83:  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/WebKitMediaSourceGStreamerPrivate.h:87:  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/WebKitMediaSourceGStreamerPrivate.h:88:  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/WebKitMediaSourceGStreamerPrivate.h:89:  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/WebKitMediaSourceGStreamerPrivate.h:90:  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.h:70:  webkit_media_src_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:202:  webkit_media_src_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:255:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 13 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Enrique Ocaña 2016-10-04 07:08:44 PDT
Comment on attachment 290598 [details]
Patch

Wait until all the patches in 157314 are ready.
Comment 4 Enrique Ocaña 2016-10-04 07:26:16 PDT
(In reply to comment #2)

> Attachment 290598 [details] did not pass style-queue:
>  
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

The style failures are on purpose, because of gobject macro machinery requirements. They aren't worth of a bug against check-webkit-style, though.
Comment 5 Xabier Rodríguez Calvar 2016-10-05 08:45:01 PDT
Comment on attachment 290598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290598&action=review

I suspect some of the members of Streams could be refcounted but I couldn't check properly yet.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:52
> +#include <wtf/MainThread.h>
> +#include <wtf/glib/GMutexLocker.h>
> +#include <wtf/glib/GUniquePtr.h>

These files are not sorted. Use style checker criteria if conflicts with mine.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:90
> +    bool allAppsrcNeedDataAfterSeek = false;

Declare this just before used

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:94
> +    int numAppsrcs = webKitMediaSrc->priv->streams.size();

This var is used only inside the if, you can move it there to just before it is used.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:194
> +// We split this out into another macro to avoid a check-webkit-style error.

I don't see this comment very useful, your call to remove it.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:213
> +    gst_element_class_set_static_metadata(eklass, "WebKit Media source element", "Source", "Handles Blob uris", "Stephane Jadaud <sjadaud@sii.fr>, Sebastian Dröge <sebastian@centricular.com>");

Might it be time to add yourself here?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:216
> +    /* Allows setting the uri using the 'location' property, which is used
> +     * for example by gst_element_make_from_uri() */

C++ comments, please period at the end.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:219
> +        g_param_spec_string("location", "location", "Location to read from", 0,

0 -> nullptr

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:299
> +        gst_uri_handler_set_uri(reinterpret_cast<GstURIHandler*>(source), g_value_get_string(value), 0);

0 -> nullptr

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:336
> +    WebKitMediaSrcPrivate* priv = source->priv;
> +    priv->asyncStart = true;

For once access, you can avoid the variable.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:353
> +    GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;

You can declare this later and please use a full name, "result" maybe.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:368
> +        GST_DEBUG_OBJECT(source, "State change failed");

GST_WARNING_OBJECT?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:533
> +        gst_app_src_set_callbacks(GST_APP_SRC(stream->appsrc), &disabledAppsrcCallbacks, nullptr, 0);

0 -> nullptr

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:604
> +// uri handler interface.

Capital U.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:619
> +    gchar* ret;

Full name

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:630
> +    WebKitMediaSrcPrivate* priv = source->priv;

You can declare this later.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:35
> +#include <wtf/RefPtr.h>
> +#include <wtf/glib/GRefPtr.h>

Unsorted. Style checker preference.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:62
> +    GstCaps* caps;

I think these should be GRefPtr even when you have to lock to remove them in the destructor function.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:104
> +    gchar* location;

GUniquePtr
Comment 6 Enrique Ocaña 2016-10-16 12:07:58 PDT
Created attachment 291766 [details]
Patch
Comment 7 WebKit Commit Bot 2016-10-16 12:12:15 PDT
Attachment 291766 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:77:  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/WebKitMediaSourceGStreamerPrivate.h:78:  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/WebKitMediaSourceGStreamerPrivate.h:79:  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/WebKitMediaSourceGStreamerPrivate.h:80:  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/WebKitMediaSourceGStreamerPrivate.h:81:  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/WebKitMediaSourceGStreamerPrivate.h:82:  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/WebKitMediaSourceGStreamerPrivate.h:86:  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/WebKitMediaSourceGStreamerPrivate.h:87:  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/WebKitMediaSourceGStreamerPrivate.h:88:  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/WebKitMediaSourceGStreamerPrivate.h:89:  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.h:71:  webkit_media_src_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:200:  webkit_media_src_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:252:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 13 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Zan Dobersek 2016-10-21 01:18:57 PDT
Comment on attachment 291766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291766&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:405
> +            float duration;
> +            if (source->priv && source->priv->mediaPlayerPrivate && ((duration = source->priv->mediaPlayerPrivate->durationMediaTime().toDouble()) > 0)) {
> +                gst_query_set_duration(query, format, WebCore::toGstClockTime(duration));

Check for source->priv && source->priv->mediaPlayerPrivate, and then set up a `duration` variable. Use toFloat() to get a float value off of durationMediaTime(), assuming there's no existing toGstClockTime() function that operates on MediaTime objects. (There should be one in the future.)

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:475
> +    if (g_str_has_prefix(structureName, "video/") && gst_video_info_from_caps(&info, caps)) {
> +        float width, height;
> +
> +        // FIXME: Correct?.
> +        width = info.width;
> +        height = info.height * ((float) info.par_d / (float) info.par_n);
> +
> +        GST_OBJECT_LOCK(stream->parent);
> +        stream->presentationSize = WebCore::FloatSize(width, height);
> +        GST_OBJECT_UNLOCK(stream->parent);
> +    } else {
> +        GST_OBJECT_LOCK(stream->parent);
> +        stream->presentationSize = WebCore::FloatSize();
> +        GST_OBJECT_UNLOCK(stream->parent);
> +    }
> +
> +    gst_caps_ref(caps);
> +    GST_OBJECT_LOCK(stream->parent);
> +    stream->caps = adoptGRef(caps);
> +    GST_OBJECT_UNLOCK(stream->parent);

Can this go under one lock-unlock section?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:573
> +    if (!(webKitMediaSrc->priv->allTracksConfigured)) {

Parentheses overload.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:667
> +    for (Stream* stream : source->priv->streams) {
> +        if (stream->type != WebCore::Invalid)
> +            stream->sourceBuffer->setReadyForMoreSamples(true);
> +    }

Why are we locking the source object to retrieve the seekTime and mediaPlayerPrivate values, but we iterate over the source's streams without a lock?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:669
> +    mediaPlayerPrivate->notifySeekNeedsDataForTime(seekTime);

What ensures that since picking up the mediaPlayerPrivate value off of the source object, the source->priv->mediaPlayerPrivate object hasn't been cleared out and the relevant MediaPlayerPrivateGStreamerMSE object deallocated?

This is also an issue in other places, both in this and other patches.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:686
> +    GST_OBJECT_LOCK(source);
> +
> +    auto it = std::find(source->priv->streams.begin(), source->priv->streams.end(), appsrcStream);
> +    if (it == source->priv->streams.end()) {
> +        GST_OBJECT_UNLOCK(source);
> +        return;
> +    }
> +
> +    WebCore::MediaPlayerPrivateGStreamerMSE* mediaPlayerPrivate = source->priv->mediaPlayerPrivate;
> +    if (mediaPlayerPrivate && !mediaPlayerPrivate->seeking())
> +        appsrcStream->sourceBuffer->notifyReadyForMoreSamples();
> +
> +    GST_OBJECT_UNLOCK(source);

OTOH this looks a lot safer, locking for the whole duration of operations that use the passed-in WebKitMediaSource.
Comment 9 Enrique Ocaña 2016-10-21 12:35:36 PDT
(In reply to comment #8)

> > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:669
> > +    mediaPlayerPrivate->notifySeekNeedsDataForTime(seekTime);
> 
> What ensures that since picking up the mediaPlayerPrivate value off of the
> source object, the source->priv->mediaPlayerPrivate object hasn't been
> cleared out and the relevant MediaPlayerPrivateGStreamerMSE object
> deallocated?

The presence (or nullness) of mediaPlayerPrivate is managed by webKitMediaSrcSetMediaPlayerPrivate(). That function also subscribes the bus signal handler (applicationMessageCallback) when mediaPlayerPrivate isn't null, and desubscribes it when it's null. If any code is running due to a message posted to the bus, mediaPlayerPrivate is guaranteed to be non-null.

The call flow is like this: "seek-needs-data" bus message --> applicationMessageCallback() --> seekNeedsDataMainThread(). Therefore, if seekNeedsDataMainThread() is being run, that means that mediaPlayerPrivate is non-null.

On the other hand, having notifySeekNeedsDataForTime() inside the locked section isn't an option, because that call can potentially trigger a GStreamer seek and cause a deadlock in the pipeline if the lock is held. Actually, I've tried to move it inside the locked section and I *consistently get a deadlock*.
Comment 10 Enrique Ocaña 2016-10-21 14:28:12 PDT
Created attachment 292411 [details]
Patch
Comment 11 WebKit Commit Bot 2016-10-21 14:31:05 PDT
Attachment 292411 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:75:  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/WebKitMediaSourceGStreamerPrivate.h:76:  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/WebKitMediaSourceGStreamerPrivate.h:77:  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/WebKitMediaSourceGStreamerPrivate.h:78:  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/WebKitMediaSourceGStreamerPrivate.h:79:  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/WebKitMediaSourceGStreamerPrivate.h:80:  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/WebKitMediaSourceGStreamerPrivate.h:84:  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/WebKitMediaSourceGStreamerPrivate.h:85:  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/WebKitMediaSourceGStreamerPrivate.h:86:  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/WebKitMediaSourceGStreamerPrivate.h:87:  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.h:71:  webkit_media_src_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:200:  webkit_media_src_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:252:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 13 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Enrique Ocaña 2016-10-21 14:41:27 PDT
Created attachment 292416 [details]
Patch
Comment 13 WebKit Commit Bot 2016-10-21 14:43:33 PDT
Attachment 292416 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:75:  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/WebKitMediaSourceGStreamerPrivate.h:76:  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/WebKitMediaSourceGStreamerPrivate.h:77:  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/WebKitMediaSourceGStreamerPrivate.h:78:  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/WebKitMediaSourceGStreamerPrivate.h:79:  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/WebKitMediaSourceGStreamerPrivate.h:80:  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/WebKitMediaSourceGStreamerPrivate.h:84:  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/WebKitMediaSourceGStreamerPrivate.h:85:  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/WebKitMediaSourceGStreamerPrivate.h:86:  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/WebKitMediaSourceGStreamerPrivate.h:87:  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.h:71:  webkit_media_src_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:200:  webkit_media_src_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:252:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 13 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Zan Dobersek 2016-10-24 06:00:59 PDT
Comment on attachment 292416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292416&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:149
> +    // No need to lock on webKitMediaSrc, we're on the main thread and nobody is going to remove the stream in the meantime.

Better safe than sorry?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:663
> +    GST_OBJECT_LOCK(source);
> +    MediaTime seekTime = source->priv->seekTime;
> +    WebCore::MediaPlayerPrivateGStreamerMSE* mediaPlayerPrivate = source->priv->mediaPlayerPrivate;
> +    GST_OBJECT_UNLOCK(source);
> +
> +    if (!mediaPlayerPrivate)
> +        return;
> +
> +    GST_OBJECT_LOCK(source);
> +    for (Stream* stream : source->priv->streams) {
> +        if (stream->type != WebCore::Invalid)
> +            stream->sourceBuffer->setReadyForMoreSamples(true);
> +    }
> +    GST_OBJECT_UNLOCK(source);

Do an early unlock-and-return if mediaPlayerPrivate is null. You gain nothing by doing the null-check outside the lock.
Comment 15 Enrique Ocaña 2016-10-25 02:24:07 PDT
Created attachment 292740 [details]
Patch
Comment 16 WebKit Commit Bot 2016-10-25 02:25:32 PDT
Attachment 292740 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:75:  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/WebKitMediaSourceGStreamerPrivate.h:76:  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/WebKitMediaSourceGStreamerPrivate.h:77:  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/WebKitMediaSourceGStreamerPrivate.h:78:  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/WebKitMediaSourceGStreamerPrivate.h:79:  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/WebKitMediaSourceGStreamerPrivate.h:80:  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/WebKitMediaSourceGStreamerPrivate.h:84:  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/WebKitMediaSourceGStreamerPrivate.h:85:  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/WebKitMediaSourceGStreamerPrivate.h:86:  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/WebKitMediaSourceGStreamerPrivate.h:87:  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.h:71:  webkit_media_src_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:200:  webkit_media_src_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:252:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 13 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2016-10-25 03:33:16 PDT
Comment on attachment 292740 [details]
Patch

Attachment 292740 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2364222

New failing tests:
svg/wicd/test-rightsizing-b.xhtml
Comment 18 Build Bot 2016-10-25 03:33:19 PDT
Created attachment 292744 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 19 Enrique Ocaña 2016-10-26 01:21:49 PDT
Created attachment 292896 [details]
Patch
Comment 20 WebKit Commit Bot 2016-10-26 01:27:19 PDT
Attachment 292896 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamerPrivate.h:75:  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/WebKitMediaSourceGStreamerPrivate.h:76:  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/WebKitMediaSourceGStreamerPrivate.h:77:  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/WebKitMediaSourceGStreamerPrivate.h:78:  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/WebKitMediaSourceGStreamerPrivate.h:79:  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/WebKitMediaSourceGStreamerPrivate.h:80:  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/WebKitMediaSourceGStreamerPrivate.h:84:  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/WebKitMediaSourceGStreamerPrivate.h:85:  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/WebKitMediaSourceGStreamerPrivate.h:86:  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/WebKitMediaSourceGStreamerPrivate.h:87:  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.h:71:  webkit_media_src_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:200:  webkit_media_src_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:252:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 13 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Enrique Ocaña 2016-10-26 01:46:19 PDT
Comment on attachment 292896 [details]
Patch

Clearing flags on attachment: 292896

Committed r207883: <http://trac.webkit.org/changeset/207883>
Comment 22 Enrique Ocaña 2016-10-26 01:46:30 PDT
All reviewed patches have been landed.  Closing bug.