Bug 129774 – [GStreamer] Disable video and/or audio if all tracks are deselected

• Component: Media
• Status: NEW

[Brendan Long]

When all video tracks are deselected, we should disable the video instead of picking a video at random to display.

[Brendan Long]

Created attachment 225947 [details]
Patch

[Brendan Long]

For some reason, disabling the video doesn't work. In some cases it seems to lock up, and it others the video crashes. I think this may be the same crash we see when the video resolution changes.

[Philippe Normand]

(In reply to comment #2)
> For some reason, disabling the video doesn't work. In some cases it seems to
> lock up, and it others the video crashes. I think this may be the same crash
> we see when the video resolution changes.

I fixed that crash recently. Perhaps you can update your patch?

[Brendan Long]

I was hoping to get to this today, but I've been swamped. I'll try to rebase and check this on Monday.

[Brendan Long]

Created attachment 242176 [details]
Patch

[Brendan Long]

Comment on attachment 242176 [details]
Patch

Here's an updated patch.

The good news is that this no longer crashes. The bad news is that it doesn't work very well. On every video I tried, disabling video caused the player to continue showing the last frame instead of blanking the video.

For an Ogg Theora video (http://download.blender.org/durian/trailer/sintel_trailer-480p.ogv), when I turn the video back on it, it displays a broken artifacty frame for a couple seconds before working fully (I assume this is waiting for an I-frame).

For all other video formats I tried (including this: http://download.blender.org/durian/trailer/sintel_trailer-480p.mp4), re-enabling the video doesn't work at all, and it also breaks ability to pause the video or seek.

I also tried doing a similar patch for audio and the videos just stop playing if you disable audio (presumably because GStreamer wants to use the audio clock).

I wonder if it would be better to just add a fakesrc for both audio and video, and switch to it when all of the audio or video tracks are disabled. If fakesrc doesn't work, we could also use a videotestsrc with pattern=GST_VIDEO_TEST_SRC_BLACK and an audiotestsrc with wave=GST_AUDIO_TEST_SRC_WAVE_SILENCE.

[Brendan Long]

And maybe we should check with a GStreamer developer to see if playbin's flags should work for this, or if they're only meant to be writable at startup.

[Philippe Normand]

Adding Sebastian to CC, what do you think about this? :)

[Sebastian Dröge (slomo)]

Disabling audio or video just disables the corresponding sinks. For audio the effect should be silence, for video it depends on what the sink is doing and how rendering works in the application. I guess webkit's video sink just does nothing when it gets deactivated, which then causes the last frame to be shown forever.

Instead it could blank the surface or whatever when it gets deactivated.

[Brendan Long]

So disabling the audio should work too?

[Sebastian Dröge (slomo)]

Yes, but it might mean that the pipeline clock disappears... which needs to be handled by WebKit then. It will get a CLOCK_LOST message on the bus and then needs to set the pipeline to PAUSED and PLAYING again.

[Philippe Normand]

(In reply to comment #11)
> Yes, but it might mean that the pipeline clock disappears... which needs to
> be handled by WebKit then. It will get a CLOCK_LOST message on the bus and
> then needs to set the pipeline to PAUSED and PLAYING again.

Oh nice, first time I hear about this message. Seems easy to support :)

[Sebastian Dröge (slomo)]

http://cgit.freedesktop.org/gstreamer/gstreamer/tree/tools/gst-launch.c#n587

See also handling of any other messages you forgot :)

[Brendan Long]

(In reply to comment #11)
> Yes, but it might mean that the pipeline clock disappears... which needs to
> be handled by WebKit then. It will get a CLOCK_LOST message on the bus and
> then needs to set the pipeline to PAUSED and PLAYING again.

Oh cool. I'll see if I can make that work.

[Brendan Long]

Created attachment 242201 [details]
Patch

This patch handles both audio and video, and pauses and restarts the video when the clock changes (note: as far as I can tell, the CLOCK_LOST message is only fired while we're in the PLAYING state). It works perfectly for audio, but I still can't get video to restart properly for most streams, and if we disable both audio and video, playback stops (even if we should continue playing subtitles).

[Sebastian Dröge (slomo)]

If you disable audio and video, I think we don't support rendering subtitles in playsink. That would need to be tested.

Do the other things all work outside webkit or is it a webkit specific problem?

[Philippe Normand]

Comment on attachment 242201 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.h:60
> +    void setEnabled(const char* flagName, const char* currentStreamProperty, bool enabled);

I think this could go in GStreamerUtilities and be reused in the player.

[Philippe Normand]

Comment on attachment 242201 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:95
> +void TrackPrivateBaseGStreamer::setPlaybinFlag(const char* flagName, bool enabled)

I meant this ^^ :)

[Brendan Long]

Created attachment 242212 [details]
Patch

I moved setPlaybinFlag() to GStreamerUtilities. I'll need to check this outside of WebKit sometime, but since we presumably want the file to continue playing without video and audio (just subtitles), it seems like adding fakesrc's will fix everything.

[Carlos Garcia Campos]

Comment on attachment 242212 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:167
> +    g_object_get(playbin, "flags", &flags, NULL);

Use nullptr instead of NULL.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:169
> +    if (value == (bool)(flags & flag))

Do you really need the bool cast? You should use C++ style casts instead. Maybe you can save the current flags, add/remove the new flag and then check if flags has changed before calling g_object_set.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:177
> +    g_object_set(playbin, "flags", flags, NULL);

nullptr

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:101
> +        g_object_set(m_playbin.get(), currentStreamProperty, m_index, NULL);

nullptr.

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:105
> +        g_object_get(m_playbin.get(), currentStreamProperty, &currentStream, NULL);

nullptr

[Philippe Normand]

Comment on attachment 242212 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1425
> +                LOG_MEDIA_MESSAGE("[Clock Changed] Restarting playback.\n");

No need for trailing \n

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1429
> +                LOG_MEDIA_MESSAGE("[Buffering] Restarting playback.\n");

Ditto

[Brendan Long]

Created attachment 242231 [details]
Patch

Fixed things based on reviews. The static_cast<bool> is there because 'flags & flag' isn't necessarily 1, and it seemed more readable than 'value == ((flags & flag) != 0)'. It seemed nicer to do the check as early as possible, rather than having two 'flags' variables.

[Philippe Normand]

Do you plan to land this manually or with cq? :)

[Brendan Long]

I'm not sure it's a good idea to commit this when it doesn't work very well on most files :\

[Sebastian Dröge (slomo)]

Why does it not work? I would expect it to fail independent of the file/stream.

Also how is disabling audio *and* video handled currently?


Would be good to get a list of things that fail, and why/how :) Most likely these are bugs in GStreamer that then should also be possible to reproduce with gst-plugins-base/tests/examples/playback.

[Brendan Long]

The remaining problems are:

 1. If you disable all audio and video, the video will stop playing (even if there are subtitles).
 2. After #1, the video won't play again even if you re-enable audio and video.
 3. Sometimes disabling and then re-enabling audio causes the audio to stay disabled, plus the seek bar and play/pause stop working (the buttons will change as if they were working, but they won't do anything). This seems to always happen with the MKV file I have (http://download.blender.org/durian/trailer/Sintel_Trailer1.480p.DivX_Plus_HD.mkv), but only if I disable/enable quickly in this MP4 (https://download.blender.org/durian/trailer/sintel_trailer-480p.mp4).

I'm guessing #1 is acceptable, although it would be nice to fix it. The other two might be WebKit issues. Maybe we need to do a pause/play when re-enabling audio and video? I'm not sure what's up with #3 but it seems like WebKit is getting confused.

[Brendan Long]

If anyone else wants to see what I'm working with, I have this to test with:

https://github.com/cablelabs/track-test

After running the get_files.sh script, the Sintel Local File buttons will work.

[Philippe Normand]

(In reply to Brendan Long from comment #27)
> If anyone else wants to see what I'm working with, I have this to test with:
> 
> https://github.com/cablelabs/track-test
> 
> After running the get_files.sh script, the Sintel Local File buttons will
> work.

Hey Brendan, no idea if you still read/care about this bug but that repo is gone from GitHub. Can you find it or explain what it did? I'd like to land this patch, almost 4 years late but heh...

[Philippe Normand]

Hah, luckily found it via https://github.com/cablelabs/AAA-Relocated-CableLabs-Repos

[Philippe Normand]

Comment on attachment 242231 [details]
Patch

Most of this patch needs doesn't apply anymore. I'll rework this.

[Philippe Normand]

Created attachment 334743 [details]
Patch

[EWS Watchlist]

Attachment 334743 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:903:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

Comment on attachment 334743 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:308
> +void setGstPlayFlag(GstElement* playbin, const char* flagName, bool value)

I think here value is a bad name, I think we should use enabled.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:322
> +    GST_DEBUG("Setting %s flag to %s", flagName, value ? "enabled":"disabled");

You can use boolForPrinting instead.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:75
> +void setGstPlayFlag(GstElement*, const char* flagName, bool value);

Given the name of the function, you could make value->enabled default to true.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:752
> +    GST_INFO("%s %s track with index: %lu", enabled ? "Enabling":"Disabling", flagName, index);

maybe boolForPrinting

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1894
> +                GST_DEBUG("[Clock Changed] Restarting playback.");

Seeing dots at the end of GST log is kinda weird.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1900
> +            if (m_fixingClock) {
> +                GST_DEBUG("[Clock Changed] Restarting playback.");
> +                m_fixingClock = false;
> +                changePipelineState(GST_STATE_PLAYING);
> +            } else if (didBuffering && !m_buffering && !m_paused && m_playbackRate) {
>                  GST_DEBUG("[Buffering] Restarting playback.");
>                  changePipelineState(GST_STATE_PLAYING);
>              }

I would write this in a different way:
if (m_fixingClock || (didBuffering && !m_buffering && !m_paused && m_playbackRate)) {
    GST_DEBUG("%s Restarting playback", m_fixingClock ? "[Clock changed]" : "[Buffering]");
    m_fixingClock = false;
    changePipelineState(GST_STATE_PLAYING);
}

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:140
> +    void setTrackEnabled(TrackType, unsigned index, bool enabled);

You can default enabled to true.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:749
> +    std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer;

layerBuffer is used only with GSTREAMER_GL, please move it inside it.a

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:787
> +            texture->reset(size, BitmapTexture::NoFlag);

Maybe we should comment about painting a black frame here as well.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:891
> +        WTF::GMutexLocker<GMutex> lock(m_sampleMutex);

Not for this patch, but we should use WTF::Lock and LockHolder here instead.

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.h:50
> +    void setTrackEnabled(bool);

For this kind of functions, I'm a big fan of the default to true :)

[Philippe Normand]

Comment on attachment 334743 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:75
>> +void setGstPlayFlag(GstElement*, const char* flagName, bool value);
> 
> Given the name of the function, you could make value->enabled default to true.

default value to true? But why? I'm not a fan of implicit things like that :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:752
>> +    GST_INFO("%s %s track with index: %lu", enabled ? "Enabling":"Disabling", flagName, index);
> 
> maybe boolForPrinting

No, boolForPrinting() returns either "true" or "false"

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1894
>> +                GST_DEBUG("[Clock Changed] Restarting playback.");
> 
> Seeing dots at the end of GST log is kinda weird.

We do it quite a lot though :) Anyway this is quite a small detail isn't it?

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:140
>> +    void setTrackEnabled(TrackType, unsigned index, bool enabled);
> 
> You can default enabled to true.

Again, why?

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:749
>> +    std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer;
> 
> layerBuffer is used only with GSTREAMER_GL, please move it inside it.a

Actually I can refactor the !GSTREAMER_GL code to also use this variable!

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:787
>> +            texture->reset(size, BitmapTexture::NoFlag);
> 
> Maybe we should comment about painting a black frame here as well.

Actually, this doesn't even compile :) Some changes in updateTexture() might be needed too. I'll update the patch.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #34)
> >> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:75
> >> +void setGstPlayFlag(GstElement*, const char* flagName, bool value);
> > 
> > Given the name of the function, you could make value->enabled default to true.
> 
> default value to true? But why? I'm not a fan of implicit things like that :)

I like them better, a matter of taste, if you don't like them, feel free to leave them as they are now.

> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:752
> >> +    GST_INFO("%s %s track with index: %lu", enabled ? "Enabling":"Disabling", flagName, index);
> > 
> > maybe boolForPrinting
> 
> No, boolForPrinting() returns either "true" or "false"

If you can't rewrite the message or just prefer to leave it like this, go ahead, it was just a suggestion.

> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1894
> >> +                GST_DEBUG("[Clock Changed] Restarting playback.");
> > 
> > Seeing dots at the end of GST log is kinda weird.
> 
> We do it quite a lot though :) Anyway this is quite a small detail isn't it?

I think we shouldn't and of course it is a small detail.

> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:749
> >> +    std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer;
> > 
> > layerBuffer is used only with GSTREAMER_GL, please move it inside it.a
> 
> Actually I can refactor the !GSTREAMER_GL code to also use this variable!

lol

> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:787
> >> +            texture->reset(size, BitmapTexture::NoFlag);
> > 
> > Maybe we should comment about painting a black frame here as well.
> 
> Actually, this doesn't even compile :) Some changes in updateTexture() might
> be needed too. I'll update the patch.

Ok.

[Philippe Normand]

Created attachment 334807 [details]
Patch

[EWS Watchlist]

Attachment 334807 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:920:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 334808 [details]
Patch

[EWS Watchlist]

Attachment 334808 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:920:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

Comment on attachment 334808 [details]
Patch

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

I think there are a couple of issues you would need to tackle that I am sorry that I could not detect before, but the most important one is if there is a test about this or you can create one.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:752
> +    GST_TRACE("Rendering %s frame", GST_IS_SAMPLE(m_sample.get()) ? "valid":"black");

Nit: I think you would leave a space around :

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:756
> +        if (UNLIKELY(!frameHolder->isValid()))
> +            return;

I think we should either at least GST_WARNING here or default to black frame. Maybe you can begin with a bool couldPaintFrame = false, set it to true just after painting and paint the black frame if (!couldPaintFrame).

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:770
> +

Nit: I think you can remove this line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> +    bool validFrame = getSampleVideoInfo(m_sample.get(), videoInfo);

This variable should be called something like isFrameValid.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:879
> +    // FIXME: Flushing works correctly only for the gst-gl code

Please, add a reference to this bug.

[Philippe Normand]

Created attachment 342928 [details]
Patch

[Philippe Normand]

(In reply to Philippe Normand from comment #41)
> Created attachment 342928 [details]
> Patch

WIP version. Please don't review!

[EWS Watchlist]

Attachment 342928 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:963:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:974:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:789:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:790:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:791:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:796:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 6 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 342928 [details]
Patch

Attachment 342928 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8231878

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 342938 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Philippe Normand]

Created attachment 378060 [details]
Patch

[Philippe Normand]

Created attachment 378061 [details]
Patch

[Philippe Normand]

Created attachment 378062 [details]
Patch

[Philippe Normand]

Created attachment 378063 [details]
Patch

[Philippe Normand]

Created attachment 378066 [details]
Patch