When all video tracks are deselected, we should disable the video instead of picking a video at random to display.
Created attachment 225947 [details] Patch
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.
(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?
I was hoping to get to this today, but I've been swamped. I'll try to rebase and check this on Monday.
Created attachment 242176 [details] Patch
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.
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.
Adding Sebastian to CC, what do you think about this? :)
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.
So disabling the audio should work too?
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.
(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 :)
http://cgit.freedesktop.org/gstreamer/gstreamer/tree/tools/gst-launch.c#n587 See also handling of any other messages you forgot :)
(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.
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).
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?
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.
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 ^^ :)
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.
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, ¤tStream, NULL); nullptr
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
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.
Do you plan to land this manually or with cq? :)
I'm not sure it's a good idea to commit this when it doesn't work very well on most files :\
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.
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.
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.
(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...
Hah, luckily found it via https://github.com/cablelabs/AAA-Relocated-CableLabs-Repos
Comment on attachment 242231 [details] Patch Most of this patch needs doesn't apply anymore. I'll rework this.
Created attachment 334743 [details] Patch
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.
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 :)
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.
(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.
Created attachment 334807 [details] Patch
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.
Created attachment 334808 [details] Patch
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.
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.
Created attachment 342928 [details] Patch
(In reply to Philippe Normand from comment #41) > Created attachment 342928 [details] > Patch WIP version. Please don't review!
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.
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
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
Created attachment 378060 [details] Patch
Created attachment 378061 [details] Patch
Created attachment 378062 [details] Patch
Created attachment 378063 [details] Patch
Created attachment 378066 [details] Patch