Bug 211718 - Introduce a RealtimeMediaSource video sample observer
Summary: Introduce a RealtimeMediaSource video sample observer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-11 03:10 PDT by youenn fablet
Modified: 2020-05-12 04:46 PDT (History)
12 users (show)

See Also:


Attachments
Patch (49.62 KB, patch)
2020-05-11 03:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.78 KB, patch)
2020-05-11 07:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (54.98 KB, patch)
2020-05-12 02:30 PDT, youenn fablet
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 2020-05-11 03:10:19 PDT
Introduce a RealtimeMediaSource video sample observer
Comment 1 youenn fablet 2020-05-11 03:17:32 PDT
Created attachment 399008 [details]
Patch
Comment 2 youenn fablet 2020-05-11 07:22:42 PDT
Created attachment 399017 [details]
Patch
Comment 3 Eric Carlson 2020-05-11 09:24:20 PDT
Comment on attachment 399017 [details]
Patch

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

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107
>      if (m_audioSource)
>          m_audioSource->removeAudioSampleObserver(*this);
> +    if (m_videoSource)
> +        m_videoSource->removeVideoSampleObserver(*this);

Maybe add a FIXME about removing these, since they should never be necessary?

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210
> +    auto locker = holdLock(m_videoSampleObserversLock);
> +    for (auto* observer : m_videoSampleObservers)
> +        observer->videoSampleAvailable(mediaSample);

Clients may perform an arbitrary amount of processing during a callback, so holding the lock for the duration of this loop might be asking for trouble.
Comment 4 Philippe Normand 2020-05-11 10:33:48 PDT
Thanks for keeping the GStreamer bits compiling!
Comment 5 youenn fablet 2020-05-11 10:49:26 PDT
(In reply to Philippe Normand from comment #4)
> Thanks for keeping the GStreamer bits compiling!

np, note that the following step might be to call videoSampleAvailable from a background thread.
While this may not break compilation, this might require some GTK changes.
Comment 6 youenn fablet 2020-05-11 10:55:47 PDT
(In reply to Eric Carlson from comment #3)
> Comment on attachment 399017 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399017&action=review
> 
> > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107
> >      if (m_audioSource)
> >          m_audioSource->removeAudioSampleObserver(*this);
> > +    if (m_videoSource)
> > +        m_videoSource->removeVideoSampleObserver(*this);
> 
> Maybe add a FIXME about removing these, since they should never be necessary?

I added them as a just-in-case, maybe we should just keep the ASSERT.
The child class should really do it since otherwise, a half-destructed object might still receive samples. 

> > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210
> > +    auto locker = holdLock(m_videoSampleObserversLock);
> > +    for (auto* observer : m_videoSampleObservers)
> > +        observer->videoSampleAvailable(mediaSample);
> 
> Clients may perform an arbitrary amount of processing during a callback, so
> holding the lock for the duration of this loop might be asking for trouble.

This is the same design as for audio samples.
I think it is fine like it is since receiving video samples should not trigger adding/removing observers, and hopefully these actions will be in different threads.
Comment 7 youenn fablet 2020-05-11 11:36:15 PDT
Comment on attachment 399017 [details]
Patch

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

>>> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107
>>> +        m_videoSource->removeVideoSampleObserver(*this);
>> 
>> Maybe add a FIXME about removing these, since they should never be necessary?
> 
> I added them as a just-in-case, maybe we should just keep the ASSERT.
> The child class should really do it since otherwise, a half-destructed object might still receive samples.

The child class destructor should do that early on otherwise, the object, while half destructed, might receive samples to process.
I'll add a comment.

>>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210
>>> +        observer->videoSampleAvailable(mediaSample);
>> 
>> Clients may perform an arbitrary amount of processing during a callback, so holding the lock for the duration of this loop might be asking for trouble.
> 
> This is the same design as for audio samples.
> I think it is fine like it is since receiving video samples should not trigger adding/removing observers, and hopefully these actions will be in different threads.

This lock is locked in add/remove observers which should happen in the main thread while this one should happen in a background thread.
Normally, videoSampleAvailable should never synchronously add/remove observers.
Comment 8 Víctor M. Jáquez L. 2020-05-12 00:22:22 PDT
(In reply to youenn fablet from comment #5)
> (In reply to Philippe Normand from comment #4)
> > Thanks for keeping the GStreamer bits compiling!
> 
> np, note that the following step might be to call videoSampleAvailable from
> a background thread.
> While this may not break compilation, this might require some GTK changes.

Indeed, thanks. I'll look this patch today.
Comment 9 youenn fablet 2020-05-12 02:30:10 PDT
Created attachment 399111 [details]
Patch for landing
Comment 10 EWS 2020-05-12 04:45:30 PDT
Committed r261553: <https://trac.webkit.org/changeset/261553>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399111 [details].
Comment 11 Radar WebKit Bug Importer 2020-05-12 04:46:16 PDT
<rdar://problem/63131874>