Bug 194326 – [GStreamer] Mock GStreamer realtime sources should keep a Ref of their mock realtime media sources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Currently MockGStreamerAudioCaptureSource::m_wrappedSource is a std::unique_ptr<RealtimeMediaSource>.
Instead it should be a Ref<RealtimeMediaSource> as m_wrappedSource can be refed in some code but destroyed when MockGStreamerAudioCaptureSource is deleted even though being refed elsewhere.
Ditto for MockGStreamerVideoCaptureSource

[Thibault Saunier]

How can it be reffed somewhere else? afair it is fully internal to the MockGStreamerAudioCaptureSource

[youenn fablet]

(In reply to Thibault Saunier from comment #1)
> How can it be reffed somewhere else? afair it is fully internal to the
> MockGStreamerAudioCaptureSource

It might ref itself for instance, or the implementation might pass a & to some other code that will keep a Ref<>.

Any class that is RefCounted<>/ThreadSafeRefCounted<> should be kept through Ref/RefPtr.
Constructors of such classes are often private/protected and a static create method is used instead.

[Thibault Saunier]

Created attachment 373915 [details]
Patch

[Philippe Normand]

Comment on attachment 373915 [details]
Patch

not needed for AudioCaptureSource?

[Thibault Saunier]

Created attachment 373917 [details]
Patch

[Thibault Saunier]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 373915 [details]
> Patch
> 
> not needed for AudioCaptureSource?

Fixed.

[Philippe Normand]

WPE EWS doesn't like this :)

[Thibault Saunier]

Created attachment 373920 [details]
Patch

[Thibault Saunier]

(In reply to Philippe Normand from comment #7)
> WPE EWS doesn't like this :)

Yeah, comited too fast, sorry about that. Should be better now :-)

[Michael Catanzaro]

(In reply to youenn fablet from comment #2)
> Constructors of such classes are often private/protected and a static create
> method is used instead.

This is the important missing piece! You should fix this too.

[Thibault Saunier]

(In reply to Michael Catanzaro from comment #10)
> (In reply to youenn fablet from comment #2)
> > Constructors of such classes are often private/protected and a static create
> > method is used instead.
> 
> This is the important missing piece! You should fix this too.

I do not think it is important in that case, the wrapping class is private and fully controlled by the GStreamer mock.

Adding it so it matches usual usage of that RefCounted classes.

[Thibault Saunier]

Created attachment 373923 [details]
Patch

[youenn fablet]

Comment on attachment 373923 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerAudioCaptureSource.cpp:134
> +        , m_src(nullptr)

GRefPtr<GstElement> might do like RefPtr<>.
Is m_src(nullptr) needed?

> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerAudioCaptureSource.h:44
> +    Ref<RealtimeMediaSource> m_wrappedSource;

Do you need m_wrappedSource to be a RealtimeMediaSource?
Can it be a WrappedMockRealtimeAudioSource so that you remove the above static_cast?

Can we separate methods from members declarations?

> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.h:46
> +    Ref<RealtimeMediaSource> m_wrappedSource;

We usually separate methods from members.

[youenn fablet]

Comment on attachment 373923 [details]
Patch

I think the patch is good to go with the proposed style improvements.

[Thibault Saunier]

(In reply to youenn fablet from comment #13)
> Comment on attachment 373923 [details]
> Patch
> > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerAudioCaptureSource.h:44
> > +    Ref<RealtimeMediaSource> m_wrappedSource;
> 
> Do you need m_wrappedSource to be a RealtimeMediaSource?
> Can it be a WrappedMockRealtimeAudioSource so that you remove the above
> static_cast?

Well, I would rather have the `WrappedMockRealtimeAudioSource` hidden in the C file than needing to have a declaration in the header.

--

Addressing the other comments.

[youenn fablet]

> Well, I would rather have the `WrappedMockRealtimeAudioSource` hidden in the
> C file than needing to have a declaration in the header.

You can probably forward declare WrappedMockRealtimeAudioSource in the header.

[Thibault Saunier]

(In reply to youenn fablet from comment #16)
> > Well, I would rather have the `WrappedMockRealtimeAudioSource` hidden in the
> > C file than needing to have a declaration in the header.
> 
> You can probably forward declare WrappedMockRealtimeAudioSource in the
> header.

Just typdefing it doesn't cut it afaics as I need need to make protected methods public in the wrapper subclass, and that doesn't seem to work.

[Thibault Saunier]

Created attachment 373932 [details]
Patch

[youenn fablet]

(In reply to Thibault Saunier from comment #17)
> (In reply to youenn fablet from comment #16)
> > > Well, I would rather have the `WrappedMockRealtimeAudioSource` hidden in the
> > > C file than needing to have a declaration in the header.
> > 
> > You can probably forward declare WrappedMockRealtimeAudioSource in the
> > header.
> 
> Just typdefing it doesn't cut it afaics as I need need to make protected
> methods public in the wrapper subclass, and that doesn't seem to work.

Unfortunate, I guess you could have a getter that returns a RealtimeMediaSource&.

[Thibault Saunier]

(In reply to youenn fablet from comment #19)
> (In reply to Thibault Saunier from comment #17)
> > (In reply to youenn fablet from comment #16)
> > > > Well, I would rather have the `WrappedMockRealtimeAudioSource` hidden in the
> > > > C file than needing to have a declaration in the header.
> > > 
> > > You can probably forward declare WrappedMockRealtimeAudioSource in the
> > > header.
> > 
> > Just typdefing it doesn't cut it afaics as I need need to make protected
> > methods public in the wrapper subclass, and that doesn't seem to work.
> 
> Unfortunate, I guess you could have a getter that returns a
> RealtimeMediaSource&.

That would work, why do you think it is better than static_cast'ing though?

[youenn fablet]

> That would work, why do you think it is better than static_cast'ing though?

Upcasting is safe and conceptually it seems better since you need the member to be of a more precise type.
In this particular case, it is one downcasting and it probably does not create a big risk so it is probably fine either way.

[youenn fablet]

Comment on attachment 373932 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerAudioCaptureSource.h:43
>      void videoSampleAvailable(MediaSample&) override { };

Should probably be final as well.

> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.h:48
>      void videoSampleAvailable(MediaSample&) override;

Ditto.

[Thibault Saunier]

Created attachment 373963 [details]
Patch for landing

[Thibault Saunier]

(In reply to youenn fablet from comment #22)
> Comment on attachment 373932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373932&action=review
> 
> > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerAudioCaptureSource.h:43
> >      void videoSampleAvailable(MediaSample&) override { };
> 
> Should probably be final as well.
Done

> > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.h:48
> >      void videoSampleAvailable(MediaSample&) override;
> 
> Ditto.

Done, also for the audio case.

(In reply to youenn fablet from comment #21)
> > That would work, why do you think it is better than static_cast'ing though?
> 
> Upcasting is safe and conceptually it seems better since you need the member
> to be of a more precise type.
> In this particular case, it is one downcasting and it probably does not
> create a big risk so it is probably fine either way.

I ended up doing what you suggested, not sure it is much better for that particular case, but I agree it is better to upcast than to downcast in any case.

[WebKit Commit Bot]

Comment on attachment 373963 [details]
Patch for landing

Rejecting attachment 373963 [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-03', 'validate-changelog', '--check-oops', '--non-interactive', 373963, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12718981

[Thibault Saunier]

Created attachment 373966 [details]
Patch for landing

[Philippe Normand]

Now the cq seems stuck in an infinite loop. :)

[Thibault Saunier]

Created attachment 374007 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 374007 [details]
Patch for landing

Clearing flags on attachment: 374007

Committed r247407: <https://trac.webkit.org/changeset/247407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53034643>