[GStreamer] Clear out m_appsinkCaps in AppendPipeline::appsinkCapsChanged() before using outPtr()
Created attachment 299157 [details] Patch
Comment on attachment 299157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299157&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:605 > + m_appsinkCaps = nullptr; I'm not sure that's the proper way to fix the bug. Have a look at how was the original code[1] before the migration to GRefPtr() and to the behaviour of gst_caps_replace()[2]. The function is thought to replace one pointer by another, automatically onreffing the old caps and reffing the new ones. Maybe the right fix would be to use raw pointers again? [1] https://github.com/eocanha/webkit/blob/mse-backport-rebased-20160414/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerMSE.cpp#L1652 [2] https://developer.gnome.org/gstreamer/1.0/gstreamer-GstCaps.html#gst-caps-replace
Comment on attachment 299157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299157&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:605 >> + m_appsinkCaps = nullptr; > > I'm not sure that's the proper way to fix the bug. Have a look at how was the original code[1] before the migration to GRefPtr() and to the behaviour of gst_caps_replace()[2]. The function is thought to replace one pointer by another, automatically onreffing the old caps and reffing the new ones. Maybe the right fix would be to use raw pointers again? > > [1] https://github.com/eocanha/webkit/blob/mse-backport-rebased-20160414/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerMSE.cpp#L1652 > [2] https://developer.gnome.org/gstreamer/1.0/gstreamer-GstCaps.html#gst-caps-replace It's not a bug, this works around the assert in GRefPtr<>::outPtr(). IMO the assert is valid, but this specific usage of it isn't. Using raw pointers doesn't fix anything, it disables a much more simpler control of an object.
(In reply to comment #3) > It's not a bug, this works around the assert in GRefPtr<>::outPtr(). IMO the > assert is valid, but this specific usage of it isn't. > ... but this specific usage of GRefPtr<>::outPtr() isn't.*
Comment on attachment 299157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299157&action=review >>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:605 >>> + m_appsinkCaps = nullptr; >> >> I'm not sure that's the proper way to fix the bug. Have a look at how was the original code[1] before the migration to GRefPtr() and to the behaviour of gst_caps_replace()[2]. The function is thought to replace one pointer by another, automatically onreffing the old caps and reffing the new ones. Maybe the right fix would be to use raw pointers again? >> >> [1] https://github.com/eocanha/webkit/blob/mse-backport-rebased-20160414/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerMSE.cpp#L1652 >> [2] https://developer.gnome.org/gstreamer/1.0/gstreamer-GstCaps.html#gst-caps-replace > > It's not a bug, this works around the assert in GRefPtr<>::outPtr(). IMO the assert is valid, but this specific usage of it isn't. > > Using raw pointers doesn't fix anything, it disables a much more simpler control of an object. I think this at least deserves a comment, explaining that gst_caps_replace does the unref. However, maybe we should do what we did for GUniqueOutPtr and replace the assert with a clear(). That way this is done automatically here and everywhere else and we avoid similar problems in the future.
Comment on attachment 299157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299157&action=review >>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:605 >>>> + m_appsinkCaps = nullptr; >>> >>> I'm not sure that's the proper way to fix the bug. Have a look at how was the original code[1] before the migration to GRefPtr() and to the behaviour of gst_caps_replace()[2]. The function is thought to replace one pointer by another, automatically onreffing the old caps and reffing the new ones. Maybe the right fix would be to use raw pointers again? >>> >>> [1] https://github.com/eocanha/webkit/blob/mse-backport-rebased-20160414/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerMSE.cpp#L1652 >>> [2] https://developer.gnome.org/gstreamer/1.0/gstreamer-GstCaps.html#gst-caps-replace >> >> It's not a bug, this works around the assert in GRefPtr<>::outPtr(). IMO the assert is valid, but this specific usage of it isn't. >> >> Using raw pointers doesn't fix anything, it disables a much more simpler control of an object. > > I think this at least deserves a comment, explaining that gst_caps_replace does the unref. However, maybe we should do what we did for GUniqueOutPtr and replace the assert with a clear(). That way this is done automatically here and everywhere else and we avoid similar problems in the future. I agree with Carlos here. Let's bring consistency between GRefPtr and GUniquePtr.
(In reply to comment #2) > Comment on attachment 299157 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299157&action=review > > > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:605 > > + m_appsinkCaps = nullptr; > > I'm not sure that's the proper way to fix the bug. Have a look at how was > the original code[1] before the migration to GRefPtr() and to the behaviour > of gst_caps_replace()[2]. The function is thought to replace one pointer by > another, automatically onreffing the old caps and reffing the new ones. > Maybe the right fix would be to use raw pointers again? > > [1] > https://github.com/eocanha/webkit/blob/mse-backport-rebased-20160414/Source/ > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerMSE.cpp#L1652 > [2] > https://developer.gnome.org/gstreamer/1.0/gstreamer-GstCaps.html#gst-caps- > replace OK, let's try raw pointers again in order to keep the atomic replacement.
Created attachment 299241 [details] Patch
*** Bug 167199 has been marked as a duplicate of this bug. ***
Comment on attachment 299241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299241&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:611 > if (m_playerPrivate && previousCapsWereNull) if (m_playerPrivate && !currentCaps) And remove the variable declaration from above
Comment on attachment 299241 [details] Patch Please write a comment on the code because it looks like we are doing things more complicated than what they are for no reason.
Enrique, do you recall why gst_caps_replace is needed here?
Comment on attachment 299241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299241&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:611 >> if (m_playerPrivate && previousCapsWereNull) > > if (m_playerPrivate && !currentCaps) > > And remove the variable declaration from above That's not correct -- if the gst_caps_replace() call succeeded, then currentCaps will now hold the same pointer as the caps GRefPtr<>.
(In reply to comment #12) > Enrique, do you recall why gst_caps_replace is needed here? At the moment (using raw pointers) it was the best way to automate the operation of unreffing the old caps and reffing the new ones. Now that I think about it, the smart pointer behaviour already do the job of gst_caps_replace(), so we could just do something like this: GRefPtr<GstPad> pad = adoptGRef(gst_element_get_static_pad(m_appsink.get(), "sink")); GRefPtr<GstCaps> newCaps = adoptGRef(gst_pad_get_current_caps(pad.get())); GRefPtr<GstCaps> currentCaps = m_appsinkCaps; if (!newCaps) return; // This means that we're right after a new track has appeared. Otherwise, it's a caps change inside the same track. bool previousCapsWereNull = !m_appsinkCaps; m_appsinkCaps = newCaps; if (currentCaps != newCaps) { if (m_playerPrivate && previousCapsWereNull) m_playerPrivate->trackDetected(this, m_oldTrack, m_track); didReceiveInitializationSegment(); gst_element_set_state(m_pipeline.get(), GST_STATE_PLAYING); }
(In reply to comment #13) > That's not correct -- if the gst_caps_replace() call succeeded, then > currentCaps will now hold the same pointer as the caps GRefPtr<>. Indeed, how could I miss that?
(In reply to comment #14) > Now that I think about it, the smart pointer behaviour already do the job of > gst_caps_replace(), so we could just do something like this: This would be my preference.
Created attachment 299347 [details] Patch
Comment on attachment 299347 [details] Patch Attachment 299347 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2920646 New failing tests: media/modern-media-controls/media-controller/media-controller-auto-hide-mouse-leave-after-play.html
Created attachment 299349 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 299347 [details] Patch Clearing flags on attachment: 299347 Committed r211026: <http://trac.webkit.org/changeset/211026>
All reviewed patches have been landed. Closing bug.