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
How can it be reffed somewhere else? afair it is fully internal to the MockGStreamerAudioCaptureSource
(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.
Created attachment 373915 [details] Patch
Comment on attachment 373915 [details] Patch not needed for AudioCaptureSource?
Created attachment 373917 [details] Patch
(In reply to Philippe Normand from comment #4) > Comment on attachment 373915 [details] > Patch > > not needed for AudioCaptureSource? Fixed.
WPE EWS doesn't like this :)
Created attachment 373920 [details] Patch
(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 :-)
(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.
(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.
Created attachment 373923 [details] Patch
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 on attachment 373923 [details] Patch I think the patch is good to go with the proposed style improvements.
(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.
> 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.
(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.
Created attachment 373932 [details] Patch
(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&.
(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?
> 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 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.
Created attachment 373963 [details] Patch for landing
(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 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
Created attachment 373966 [details] Patch for landing
Now the cq seems stuck in an infinite loop. :)
Created attachment 374007 [details] Patch for landing
Comment on attachment 374007 [details] Patch for landing Clearing flags on attachment: 374007 Committed r247407: <https://trac.webkit.org/changeset/247407>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53034643>