Bug 167165 – [GStreamer] Clear out m_appsinkCaps in AppendPipeline::appsinkCapsChanged() before using outPtr()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GStreamer] Clear out m_appsinkCaps in AppendPipeline::appsinkCapsChanged() before using outPtr()

[Zan Dobersek]

Created attachment 299157 [details]
Patch

[Enrique Ocaña]

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

[Zan Dobersek]

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.

[Zan Dobersek]

(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.*

[Carlos Garcia Campos]

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.

[Xabier Rodríguez Calvar]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Created attachment 299241 [details]
Patch

[Carlos Garcia Campos]

*** Bug 167199 has been marked as a duplicate of this bug. ***

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

Enrique, do you recall why gst_caps_replace is needed here?

[Zan Dobersek]

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<>.

[Enrique Ocaña]

(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);
    }

[Xabier Rodríguez Calvar]

(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?

[Xabier Rodríguez Calvar]

(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.

[Zan Dobersek]

Created attachment 299347 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Zan Dobersek]

Comment on attachment 299347 [details]
Patch

Clearing flags on attachment: 299347

Committed r211026: <http://trac.webkit.org/changeset/211026>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.