Bug 191631 – [GStreamer][WebRTC] Add support for sending silence or silencing an incoming track

• Component: New Bugs
• Status: RESOLVED FIXED

[Thibault Saunier]

[GStreamer][WebRTC] Add support for sending silence or silencing an incoming track

[Thibault Saunier]

Created attachment 354796 [details]
Patch

[Thibault Saunier]

Created attachment 354799 [details]
Patch

[Thibault Saunier]

Created attachment 354805 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 354805 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:127
> +    GstMappedBuffer outmap(outbuf.get(), GST_MAP_READ);
> +    if (isSilenced())
> +        gst_audio_format_fill_silence(m_outputStreamDescription->getInfo()->finfo, outmap.data(), outmap.size());
> +    else {
> +        GstMappedBuffer inmap(inbuf.get(), GST_MAP_READ);

Both inmap and outmap should have more meaningful names and properly capitalized.

It looks like the flags for outmap should be WRITE instead of READ.

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:131
> +        if (!gst_audio_converter_samples(m_sampleConverter, static_cast<GstAudioConverterFlags>(0), in, inChunkSampleCount, out, outChunkSampleCount)) {

I was checking the m_sampleConverter in the .h and saw that it's a plain pointer. It should be turned into a GUniquePtr not now but at some point.

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:142
> +    sendAudioFrames(outmap.data(),
> +        LibWebRTCAudioFormat::sampleSize,
> +        static_cast<int>(m_outputStreamDescription->sampleRate()),
> +        static_cast<int>(m_outputStreamDescription->numberOfChannels()),
> +        outChunkSampleCount);

It won't probably fit in a single line but you can probably make two.

And I guess sendAudioFrames (which I couldn't find anywhere, I assume it's libwebrtc code) is going to copy that that from the mapped thing, otherwise nasty things can happen when it is unmapped, right?

[Thibault Saunier]

Created attachment 354917 [details]
Patch for landing

[Thibault Saunier]

Created attachment 354918 [details]
Patch for landing

[Thibault Saunier]

(In reply to Xabier Rodríguez Calvar from comment #4)
> Comment on attachment 354805 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354805&action=review
> 
> > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:127
> > +    GstMappedBuffer outmap(outbuf.get(), GST_MAP_READ);
> > +    if (isSilenced())
> > +        gst_audio_format_fill_silence(m_outputStreamDescription->getInfo()->finfo, outmap.data(), outmap.size());
> > +    else {
> > +        GstMappedBuffer inmap(inbuf.get(), GST_MAP_READ);
> 
> Both inmap and outmap should have more meaningful names and properly
> capitalized.

Well, inMap/outMap sounds meaningful in the context, if you have better name just suggest please :-)

> It looks like the flags for outmap should be WRITE instead of READ.

Indeed, good catch.

> > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:131
> > +        if (!gst_audio_converter_samples(m_sampleConverter, static_cast<GstAudioConverterFlags>(0), in, inChunkSampleCount, out, outChunkSampleCount)) {
> 
> I was checking the m_sampleConverter in the .h and saw that it's a plain
> pointer. It should be turned into a GUniquePtr not now but at some point.

Indeed, would make sense.

> > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:142
> > +    sendAudioFrames(outmap.data(),
> > +        LibWebRTCAudioFormat::sampleSize,
> > +        static_cast<int>(m_outputStreamDescription->sampleRate()),
> > +        static_cast<int>(m_outputStreamDescription->numberOfChannels()),
> > +        outChunkSampleCount);
> 
> It won't probably fit in a single line but you can probably make two.
> 
> And I guess sendAudioFrames (which I couldn't find anywhere, I assume it's
> libwebrtc code) is going to copy that that from the mapped thing, otherwise
> nasty things can happen when it is unmapped, right?

sendAudioFrames was added a few days ago, I guess it is why your couldn't find it, and
yes, that function is not giving ownership to the callee

[WebKit Commit Bot]

Comment on attachment 354918 [details]
Patch for landing

Rejecting attachment 354918 [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-01', 'apply-attachment', '--no-update', '--non-interactive', 354918, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=354918&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=191631&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 354918 from bug 191631.
Fetching: https://bugs.webkit.org/attachment.cgi?id=354918
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 3 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/ChangeLog.rej
patching file Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingAudioSourceLibWebRTC.cpp
patching file Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

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

[Xabier Rodríguez Calvar]

Comment on attachment 354918 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:20
> -        Reviewed by NOBODY (OOPS!).
> +        Reviewed by Xabier Rodriguez-Calvar.

Big ooops!

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:123
> +    GstMappedBuffer outMap(outBuffer.get(), GST_MAP_WRITE);

I would say inMappedBuffer and out

[Thibault Saunier]

Created attachment 354924 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354924 [details]
Patch for landing

Clearing flags on attachment: 354924

Committed r238224: <https://trac.webkit.org/changeset/238224>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46094436>