Bug 194326 - [GStreamer] Mock GStreamer realtime sources should keep a Ref of their mock realtime media sources
Summary: [GStreamer] Mock GStreamer realtime sources should keep a Ref of their mock r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thibault Saunier
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-05 19:30 PST by youenn fablet
Modified: 2019-07-12 17:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.88 KB, patch)
2019-07-11 05:42 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (4.83 KB, patch)
2019-07-11 06:21 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2019-07-11 06:45 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (7.61 KB, patch)
2019-07-11 08:41 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (7.80 KB, patch)
2019-07-11 11:30 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (9.72 KB, patch)
2019-07-11 15:09 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (10.38 KB, patch)
2019-07-11 15:33 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (10.38 KB, patch)
2019-07-12 06:27 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-02-05 19:30:28 PST
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
Comment 1 Thibault Saunier 2019-02-05 19:33:40 PST
How can it be reffed somewhere else? afair it is fully internal to the MockGStreamerAudioCaptureSource
Comment 2 youenn fablet 2019-02-05 19:39:26 PST
(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.
Comment 3 Thibault Saunier 2019-07-11 05:42:35 PDT
Created attachment 373915 [details]
Patch
Comment 4 Philippe Normand 2019-07-11 06:07:14 PDT
Comment on attachment 373915 [details]
Patch

not needed for AudioCaptureSource?
Comment 5 Thibault Saunier 2019-07-11 06:21:03 PDT
Created attachment 373917 [details]
Patch
Comment 6 Thibault Saunier 2019-07-11 06:21:17 PDT
(In reply to Philippe Normand from comment #4)
> Comment on attachment 373915 [details]
> Patch
> 
> not needed for AudioCaptureSource?

Fixed.
Comment 7 Philippe Normand 2019-07-11 06:35:51 PDT
WPE EWS doesn't like this :)
Comment 8 Thibault Saunier 2019-07-11 06:45:00 PDT
Created attachment 373920 [details]
Patch
Comment 9 Thibault Saunier 2019-07-11 06:45:34 PDT
(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 :-)
Comment 10 Michael Catanzaro 2019-07-11 08:04:04 PDT
(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.
Comment 11 Thibault Saunier 2019-07-11 08:40:37 PDT
(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.
Comment 12 Thibault Saunier 2019-07-11 08:41:04 PDT
Created attachment 373923 [details]
Patch
Comment 13 youenn fablet 2019-07-11 09:22:15 PDT
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.
Comment 14 youenn fablet 2019-07-11 09:49:43 PDT
Comment on attachment 373923 [details]
Patch

I think the patch is good to go with the proposed style improvements.
Comment 15 Thibault Saunier 2019-07-11 10:47:53 PDT
(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.
Comment 16 youenn fablet 2019-07-11 11:18:34 PDT
> 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.
Comment 17 Thibault Saunier 2019-07-11 11:26:03 PDT
(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.
Comment 18 Thibault Saunier 2019-07-11 11:30:22 PDT
Created attachment 373932 [details]
Patch
Comment 19 youenn fablet 2019-07-11 11:35:39 PDT
(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&.
Comment 20 Thibault Saunier 2019-07-11 12:14:48 PDT
(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?
Comment 21 youenn fablet 2019-07-11 14:10:57 PDT
> 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.
Comment 22 youenn fablet 2019-07-11 14:12:45 PDT
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.
Comment 23 Thibault Saunier 2019-07-11 15:09:36 PDT
Created attachment 373963 [details]
Patch for landing
Comment 24 Thibault Saunier 2019-07-11 15:12:46 PDT
(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.
Comment 25 WebKit Commit Bot 2019-07-11 15:27:22 PDT
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
Comment 26 Thibault Saunier 2019-07-11 15:33:33 PDT
Created attachment 373966 [details]
Patch for landing
Comment 27 Philippe Normand 2019-07-12 02:08:59 PDT
Now the cq seems stuck in an infinite loop. :)
Comment 28 Thibault Saunier 2019-07-12 06:27:18 PDT
Created attachment 374007 [details]
Patch for landing
Comment 29 WebKit Commit Bot 2019-07-12 17:05:24 PDT
Comment on attachment 374007 [details]
Patch for landing

Clearing flags on attachment: 374007

Committed r247407: <https://trac.webkit.org/changeset/247407>
Comment 30 WebKit Commit Bot 2019-07-12 17:05:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Radar WebKit Bug Importer 2019-07-12 17:12:07 PDT
<rdar://problem/53034643>