Bug 164022 - REGRESSION(r207879-207891): [GStreamer] Introduced many layout test failures and crashes, bots exiting early
Summary: REGRESSION(r207879-207891): [GStreamer] Introduced many layout test failures ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on: 164116
Blocks: 143233 143704 143706 158925 160019 162719 163514 163515 163516 163824
  Show dependency treegraph
 
Reported: 2016-10-26 10:43 PDT by Michael Catanzaro
Modified: 2016-12-26 03:37 PST (History)
6 users (show)

See Also:


Attachments
Patch for landing (9.90 KB, patch)
2016-10-27 04:48 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (42.56 KB, patch)
2016-12-05 14:01 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (38.94 KB, patch)
2016-12-12 09:29 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (40.32 KB, patch)
2016-12-13 03:50 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (40.32 KB, patch)
2016-12-13 04:12 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (40.37 KB, patch)
2016-12-13 04:16 PST, 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 Michael Catanzaro 2016-10-26 10:43:00 PDT
Layout test results for r207877:

Failed 2 jsc tests failed 131 failures 68 new passes 14 flakes 0 api tests failed, 0 crashed, 1 timed out, 22 skipped

For r207891, the next test run:

Failed 2 jsc tests failed exiting early after 16 crashes and 34 timeouts. 45946 tests run. 187 failures 35 new passes 1 api tests failed, 0 crashed, 0 timed out, 22 skipped

All of the commits in this range are GStreamer patches. I don't want to slow down your work by rolling out these patches, but I do ask that you please review and update the test expectations to make sure the bot is as happy as it was before this landed.
Comment 1 Carlos Garcia Campos 2016-10-26 22:30:04 PDT
The release bot has been exiting early since yesterday. If the problem is encrypted media, then we can simply disable it until it's ready to avoid roll outs, or more gardening work. Otherwise I'm sorry but we should probably roll out the patches. Next time, before introducing a large change like this one, run the tests before landing.
Comment 2 Enrique Ocaña 2016-10-27 02:40:09 PDT
I'm terribly sorry for the inconvenience. I'm having a look into this issue a soon as I get home (~2 hours). Anyway, probably the first step will be disabling MEDIA_SOURCE and LEGACY_ENCRYPTED_MEDIA.
Comment 3 Xabier Rodríguez Calvar 2016-10-27 02:41:10 PDT
(In reply to comment #1)
> The release bot has been exiting early since yesterday. If the problem is
> encrypted media, then we can simply disable it until it's ready to avoid
> roll outs, or more gardening work. Otherwise I'm sorry but we should
> probably roll out the patches. Next time, before introducing a large change
> like this one, run the tests before landing.

I think rolling out the patches is overkill. It should be enough with skipping MSE and EME tests for the time being.
Comment 4 Xabier Rodríguez Calvar 2016-10-27 04:32:57 PDT
I will submit a patch soon. I am removing these lines from the gtk expectations, please bear them in mind when reactiving them again.

webkit.org/b/99065 media/media-source/media-source-append-failed.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-multiple-initialization-segments.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-append-nonsync-sample-after-abort.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-duplicate-seeked.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-canplaythrough.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-duration-after-append.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-end-of-stream-buffered.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-end-of-stream.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-fudge-factor.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-monitor-source-buffers.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-overlapping-append.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-overlapping-decodetime.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-play.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-remove.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-seek-complete.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-track-enabled.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-tracks.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-video-playback-quality.html [ Skip ]
webkit.org/b/99065 media/media-source/media-source-seek-detach-crash.html [ Skip ]

webkit.org/b/143233 media/media-source/media-source-append-buffer-with-append-window.html [ Pass Failure ]

webkit.org/b/143704 media/media-source/media-source-append-media-segment-without-init.html [ Failure Pass ]

webkit.org/b/142491 media/media-source/media-source-stalled-holds-sleep-assertion.html [ Timeout ]

webkit.org/b/143706 media/media-source/media-source-end-of-stream-readyState.html [ Failure ]

webkit.org/b/158925 media/media-source/media-source-loadedmetada-with-two-sourcebuffers.html [ Timeout ]

webkit.org/b/160019 media/media-source/media-source-small-gap.html [ Timeout ]

webkit.org/b/163514 media/media-source/media-source-sequence-timestamps.html [ Failure ]
webkit.org/b/163515 media/media-source/media-source-stpp-crash.html [ Failure ]
webkit.org/b/163516 media/media-source/media-source-timeoffset.html [ Failure ]

webkit.org/b/163824 media/media-source/media-source-init-segment-duration.html [ Failure ]
Comment 5 Xabier Rodríguez Calvar 2016-10-27 04:48:36 PDT
Created attachment 293012 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-10-27 05:23:43 PDT
Comment on attachment 293012 [details]
Patch for landing

Clearing flags on attachment: 293012

Committed r207989: <http://trac.webkit.org/changeset/207989>
Comment 7 WebKit Commit Bot 2016-10-27 05:23:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Michael Catanzaro 2016-10-27 07:04:08 PDT
(In reply to comment #2)
> I'm terribly sorry for the inconvenience. I'm having a look into this issue
> a soon as I get home (~2 hours). Anyway, probably the first step will be
> disabling MEDIA_SOURCE and LEGACY_ENCRYPTED_MEDIA.

No problem, it's to be expected when landing a big new feature.

(In reply to comment #6)
> Comment on attachment 293012 [details]
> Patch for landing
> 
> Clearing flags on attachment: 293012
> 
> Committed r207989: <http://trac.webkit.org/changeset/207989>

Good job, it worked.

Note that this does hide many crashes. Enrique, you might want to look at the crash logs from the release bot to see what it was doing. They are not all the same.
Comment 9 Michael Catanzaro 2016-10-27 07:05:35 PDT
However this bug needs to stay open until the new test expectation is removed:

webkit.org/b/164022 media/media-source [ Skip ]

It would be better to reference a different bug (preferably a generic "implement MSE" bug) in the expectation.
Comment 10 Enrique Ocaña 2016-12-05 14:01:27 PST
Created attachment 296187 [details]
Patch
Comment 11 Michael Catanzaro 2016-12-05 14:30:13 PST
Warning: your patch accidentally removes an old changelog entry!
Comment 12 Enrique Ocaña 2016-12-06 11:38:06 PST
(In reply to comment #11)
> Warning: your patch accidentally removes an old changelog entry!

Hmmm... I have to double-check on top of where I'm applying the branch I use to produce the patch, because there seems to be a discrepancy between how git and svn produce the diffs here.
Comment 13 Xabier Rodríguez Calvar 2016-12-08 02:54:27 PST
Comment on attachment 296187 [details]
Patch

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

> Source/WebCore/ChangeLog:53
> +        * platform/graphics/gstreamer/mse/AppendPipeline.h:
> +          New parameter.

This seems to be misaligned and the comment says nothing meaningful. Either remove it or specify it.

> Source/WebCore/ChangeLog:60
> +        New method.

Which

> Source/WebCore/ChangeLog:-25993
> -        [GStreamer] Drain query support
> -        https://bugs.webkit.org/show_bug.cgi?id=162872
> -
> -        Reviewed by Žan Doberšek.
> -
> -        Under some circumstances, GStreamer deadlocks completely during MSE seeks on OpenGL ES
> -        systems. This is because the video sink still holds samples. Proper DRAIN support
> -        fixes this issue and is also good for all the platforms, not only OpenGL ES.
> -
> -        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
> -        (WebCore::MediaPlayerPrivateGStreamerBase::triggerDrain):
> -        (WebCore::MediaPlayerPrivateGStreamerBase::drainCallback):
> -        (WebCore::MediaPlayerPrivateGStreamerBase::createVideoSink):
> -        (WebCore::MediaPlayerPrivateGStreamerBase::createVideoSinkGL): Deleted.
> -        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:
> -        * platform/graphics/gstreamer/VideoSinkGStreamer.cpp:
> -        (VideoRenderRequestScheduler::drain):
> -        (webkitVideoSinkQuery):
> -        (webkitVideoSinkEvent):
> -        (webkit_video_sink_class_init):
> -        (VideoRenderRequestScheduler::stop): Deleted.
> -        (webkitVideoSinkProposeAllocation): Deleted.
> -
> -2016-10-26  Enrique Ocaña González  <eocanha@igalia.com>
> -

Cannot remove changelog entries.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:239
> +#if USE(GSTREAMER)
> +    if (Settings::isGStreamerEnabled()) {
> +#endif
> +        PlatformMediaEngineClassName::registerMediaEngine(addMediaEngine);
> +#if USE(GSTREAMER)
> +    }
> +#endif

I was about to tell you to use a #if #else here but I realized that having just one instruction you can have the flagged if without the {.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:535
> +            m_presentationSize = WebCore::FloatSize();

Just a question: just wonder why you decided to switch some other types from float to double and you prefer to keep this one.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:733
> -        GST_ERROR("Unsupported or unknown stream type");
> -        ASSERT_NOT_REACHED();
> +        GST_ERROR("Unsupported stream type or codec");

Why do you remove the assertion?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:885
> +        gulong* probeId = g_new0(gulong, 1);
> +        *probeId = gst_pad_add_probe(demuxerSrcPad, GST_PAD_PROBE_TYPE_BUFFER, reinterpret_cast<GstPadProbeCallback>(appendPipelineDemuxerBlackHolePadProbe), nullptr, nullptr);
> +        g_object_set_data(G_OBJECT(demuxerSrcPad), "blackHoleProbeId", probeId);

Maybe we can save this memory allocation with GUINT_TO_POINTER() and GPOINTER_TO_UINT().

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1001
> +        {
> +            GUniquePtr<gchar> strcaps(gst_caps_to_string(caps.get()));
> +            GST_DEBUG("Unsupported track codec: %s", strcaps.get());
> +        }

Please, flag this under GStreamer debug being enabled. Converting caps to string is a non trivial operation we could end up saving if debug is not enabled at compile time.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:769
> +        for (const auto& pattern : supportedCodecs) {
> +            if (isCodecSupported = !fnmatch(pattern, codecData, 0))
> +                break;
> +        }

I think you don't need the { }

> Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:198
> +// Defaults to true.
> +WK_EXPORT void WKPreferencesSetGStreamerEnabled(WKPreferencesRef preferencesRef, bool enabled);
> +WK_EXPORT bool WKPreferencesGetGStreamerEnabled(WKPreferencesRef preferencesRef);
> +

Do we need to export this to the C API? Can't it be kept just for the internals?

> LayoutTests/media/media-source/media-source-resize.html:51
> +        // First resize can be 0x0 (Mac) or 640x480 (GTK+)
> +        if (video.videoWidth > 0 && video.videoHeight > 0) {
> +            testExpected('video.videoWidth', 640);
> +            testExpected('video.videoHeight', 480);
> +            endTest();
> +        }

Why do we want to stop the test here instead of waiting for the update, etc?

> LayoutTests/platform/gtk/TestExpectations:238
> +webkit.org/b/165394 media/media-source/media-source-seek-detach-crash.html [ Timeout ]

I think it is better to Skip the test if it is going to timeout, test run is faster.
Comment 14 Xabier Rodríguez Calvar 2016-12-08 04:31:58 PST
(In reply to comment #4)
> I will submit a patch soon. I am removing these lines from the gtk
> expectations, please bear them in mind when reactiving them again.
> 
> ...

And I assume that by not adding these expectations again, they work, right?
Comment 15 Enrique Ocaña 2016-12-12 06:03:52 PST
Comment on attachment 296187 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:535
>> +            m_presentationSize = WebCore::FloatSize();
> 
> Just a question: just wonder why you decided to switch some other types from float to double and you prefer to keep this one.

The attributes I changed were related to time management, where lack of precission was causing troubles in the tests. This m_presentationSize attribute is dimensions related, its current type isn't causing any problem with the tests and, what's more important, must be FloatSize to meet the requirements of the MediaSample interface.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:733
>> +        GST_ERROR("Unsupported stream type or codec");
> 
> Why do you remove the assertion?

Because before the "unsupported stream type or codec" condition wasn't reported to the upper layer and we didn't know what to do with it (thus the assertion), but now it's "supported" (we just don't generate any track and let SourceBuffer do its job and trigger an append error[1].

[1] https://github.com/WebKit/webkit/blob/81708ead2798a7c6b32ea99b5678d989219e2118/Source/WebCore/Modules/mediasource/SourceBuffer.cpp#L1012

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:885
>> +        g_object_set_data(G_OBJECT(demuxerSrcPad), "blackHoleProbeId", probeId);
> 
> Maybe we can save this memory allocation with GUINT_TO_POINTER() and GPOINTER_TO_UINT().

No, that can be a source of future problems. gst_pad_add_probe() returns a gulong. If we demote it to a guint to let GUINT_TO_POINTER() fit it in a pointer, we can corrupt its value. We would end up having a hard debugging time trying to find where those random "invalid probe id" asserts are coming from.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1001
>> +        }
> 
> Please, flag this under GStreamer debug being enabled. Converting caps to string is a non trivial operation we could end up saving if debug is not enabled at compile time.

I can do that if you really want, but this piece of code is going to be run very rarely (at most 2 or 3 times per played video, that's once per track, and only in situations where we have very bad luck and don't support the codecs). On the other hand, having the info about what's the specific failing codec can be very useful even in release builds targeted to end users.

>> Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:198
>> +
> 
> Do we need to export this to the C API? Can't it be kept just for the internals?

Yes, thanks for the hint, keeping it internal is enough. I had just followed the changes originally done for AVFoundationEnabled without really reflecting about if replicating the changes on each layer was really suitable for this case or not.

>> LayoutTests/media/media-source/media-source-resize.html:51
>> +        }
> 
> Why do we want to stop the test here instead of waiting for the update, etc?

The original (Mac) test doesn't wait for the update of the append which triggers the resize either (the second append, containing media data). They just wait for the update of the first (init segment) append because that's a natural step before wanting to start doing the second append, the one really important for them.
I've tried to change the test the least possible (actually, just adding the "if" condition). Waiting for an update after the right size is detected would imply deeper changes in the test code and potential regressions on Mac with no additional gain. After all, there are probably better tests than this one to test for the "update" event.
Comment 16 Enrique Ocaña 2016-12-12 06:24:14 PST
(In reply to comment #14)
> (In reply to comment #4)
> > I will submit a patch soon. I am removing these lines from the gtk
> > expectations, please bear them in mind when reactiving them again.
> > 
> > ...
> 
> And I assume that by not adding these expectations again, they work, right?

Yes, all the 40 LayoutTests/media/media-source tests pass except the only one I'm skipping (media-source-seek-detach-crash.html):

-------------------------

$ NUMBER_OF_PROCESSORS=8 Tools/Scripts/run-webkit-tests --gtk --no-new-test-results --fully-parallel --repeat-each=1 --iterations=1 --skipped=ignore \
media/media-source/media-source-seek-back.html \
media/media-source/media-source-video-playback-quality.html \
media/media-source/media-source-remove-crash.html \
media/media-source/media-source-sequence-timestamps.html \
media/media-source/media-source-remove.html \
media/media-source/media-source-fudge-factor.html \
media/media-source/media-source-append-media-segment-without-init.html \
media/media-source/media-source-addsourcebuffer.html \
media/media-source/media-source-sample-wrong-track-id.html \
media/media-source/media-source-duplicate-seeked.html \
media/media-source/media-source-append-failed.html \
media/media-source/media-source-stalled-holds-sleep-assertion.html \
media/media-source/media-source-canplaythrough.html \
media/media-source/media-source-overlapping-append-buffered.html \
media/media-source/media-source-init-segment-duration.html \
media/media-source/media-source-loadedmetada-with-two-sourcebuffers.html \
media/media-source/media-source-end-of-stream.html \
media/media-source/media-source-track-enabled.html \
media/media-source/media-source-append-buffer.html \
media/media-source/media-source-resize.html \
media/media-source/media-source-seek-complete.html \
media/media-source/media-source-seek-detach-crash.html \
media/media-source/media-source-overlapping-decodetime.html \
media/media-source/media-source-small-gap.html \
media/media-source/media-source-overlapping-append.html \
media/media-source/media-source-duration-after-append.html \
media/media-source/media-source-timeoffset.html \
media/media-source/media-source-delaying-load-event.html \
media/media-source/media-source-tracks.html \
media/media-source/media-source-append-nonsync-sample-after-abort.html \
media/media-source/media-source-play.html \
media/media-source/media-source-multiple-initialization-segments.html \
media/media-source/media-source-closed.html \
media/media-source/media-source-monitor-source-buffers.html \
media/media-source/media-source-fastseek.html \
media/media-source/media-source-end-of-stream-readyState.html \
media/media-source/media-source-end-of-stream-buffered.html \
media/media-source/media-source-stpp-crash.html \
media/media-source/media-source-abort-resets-parser.html \
media/media-source/media-source-append-buffer-with-append-window.html \
fast/history/page-cache-media-source-closed-2.html \
fast/history/page-cache-media-source-closed.html \
fast/history/page-cache-media-source-opened.html \
http/tests/media/media-source

[...]
Found 44 tests; running 44, skipping 0.
Running 44 tests
Running 8 WebKitTestRunners in parallel.
[44/44] media/media-source/media-source-seek-detach-crash.html failed unexpectedly (test timed out, text diff)
                        
Retrying 1 unexpected failure ...
Running 1 WebKitTestRunner.

[1/1] media/media-source/media-source-seek-detach-crash.html failed unexpectedly (test timed out, text diff)

43 tests ran as expected, 1 didn't:

Regressions: Unexpected timeouts (1)
  media/media-source/media-source-seek-detach-crash.html [ Timeout ]

-------------------------
Comment 17 Enrique Ocaña 2016-12-12 09:29:32 PST
Created attachment 296924 [details]
Patch
Comment 18 Xabier Rodríguez Calvar 2016-12-13 02:05:46 PST
Comment on attachment 296924 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:769
> +        for (const auto& pattern : supportedCodecs) {
> +            if (isCodecSupported = !fnmatch(pattern, codecData, 0))
> +                break;
> +        }

I think you don't need the { }
Comment 19 Xabier Rodríguez Calvar 2016-12-13 02:07:10 PST
(In reply to comment #15)
> >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:885
> >> +        g_object_set_data(G_OBJECT(demuxerSrcPad), "blackHoleProbeId", probeId);
> > 
> > Maybe we can save this memory allocation with GUINT_TO_POINTER() and GPOINTER_TO_UINT().
> 
> No, that can be a source of future problems. gst_pad_add_probe() returns a
> gulong. If we demote it to a guint to let GUINT_TO_POINTER() fit it in a
> pointer, we can corrupt its value. We would end up having a hard debugging
> time trying to find where those random "invalid probe id" asserts are coming
> from.

Maybe it is worth a try to investigate saving it with a guint as temporary buffer.
Comment 20 Xabier Rodríguez Calvar 2016-12-13 02:08:36 PST
And to be explicit, I acknowledge the rest of your comments.
Comment 21 Enrique Ocaña 2016-12-13 03:50:23 PST
Created attachment 297001 [details]
Patch
Comment 22 Enrique Ocaña 2016-12-13 03:56:08 PST
(In reply to comment #18)
> Comment on attachment 296924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296924&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:769
> > +        for (const auto& pattern : supportedCodecs) {
> > +            if (isCodecSupported = !fnmatch(pattern, codecData, 0))
> > +                break;
> > +        }
> 
> I think you don't need the { }

Sorry, I was going to answer this before but I forgot. The style checker doesn't like the block without braces. That's why they're there:

ERROR: Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:766:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Comment 23 Xabier Rodríguez Calvar 2016-12-13 04:04:17 PST
Comment on attachment 297001 [details]
Patch

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

Thanks! Good job!

> Source/WTF/wtf/glib/GLibUtilities.h:34
> +#define GPOINTER_TO_ULONG(p) ((gsize) (p))

I guess this meant (gulong)

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1029
> +            GST_DEBUG("Not disconnecting demuxer src pad because it wasn't linked");

If this is uncommon, this should be a GST_WARNING.

Sorry for not seeing this before.
Comment 24 Enrique Ocaña 2016-12-13 04:12:17 PST
Created attachment 297002 [details]
Patch
Comment 25 Enrique Ocaña 2016-12-13 04:16:34 PST
Created attachment 297003 [details]
Patch
Comment 26 Enrique Ocaña 2016-12-13 04:19:33 PST
(In reply to comment #23)
> Comment on attachment 297001 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297001&action=review
> 
> Thanks! Good job!
> 
> > Source/WTF/wtf/glib/GLibUtilities.h:34
> > +#define GPOINTER_TO_ULONG(p) ((gsize) (p))
> 
> I guess this meant (gulong)
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1029
> > +            GST_DEBUG("Not disconnecting demuxer src pad because it wasn't linked");
> 
> If this is uncommon, this should be a GST_WARNING.
> 
> Sorry for not seeing this before.

Both comments are addressed in https://bugs.webkit.org/attachment.cgi?id=297003

This patch should only be committed after the one from https://bugs.webkit.org/show_bug.cgi?id=164116
Comment 27 WebKit Commit Bot 2016-12-14 02:37:10 PST
Comment on attachment 297003 [details]
Patch

Rejecting attachment 297003 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 297003, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ca3281fe4fafc2384b75ad3cfb989278e9d 4bcf0ec18511bff587454250363acc9ee61bfe98 M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 1 files

Full output: http://webkit-queues.webkit.org/results/2719852
Comment 28 WebKit Commit Bot 2016-12-14 03:16:59 PST
Comment on attachment 297003 [details]
Patch

Clearing flags on attachment: 297003

Committed r209797: <http://trac.webkit.org/changeset/209797>
Comment 29 WebKit Commit Bot 2016-12-14 03:17:05 PST
All reviewed patches have been landed.  Closing bug.