Bug 191631 - [GStreamer][WebRTC] Add support for sending silence or silencing an incoming track
Summary: [GStreamer][WebRTC] Add support for sending silence or silencing an incoming ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thibault Saunier
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-14 04:28 PST by Thibault Saunier
Modified: 2018-11-15 06:40 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.04 KB, patch)
2018-11-14 04:45 PST, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (5.05 KB, patch)
2018-11-14 04:52 PST, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (4.90 KB, patch)
2018-11-14 07:15 PST, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (5.06 KB, patch)
2018-11-15 05:12 PST, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (5.29 KB, patch)
2018-11-15 05:12 PST, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (5.08 KB, patch)
2018-11-15 06:01 PST, 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 Thibault Saunier 2018-11-14 04:28:06 PST
[GStreamer][WebRTC] Add support for sending silence or silencing an incoming track
Comment 1 Thibault Saunier 2018-11-14 04:45:49 PST
Created attachment 354796 [details]
Patch
Comment 2 Thibault Saunier 2018-11-14 04:52:25 PST
Created attachment 354799 [details]
Patch
Comment 3 Thibault Saunier 2018-11-14 07:15:45 PST
Created attachment 354805 [details]
Patch
Comment 4 Xabier Rodríguez Calvar 2018-11-15 04:14:55 PST
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?
Comment 5 Thibault Saunier 2018-11-15 05:12:01 PST
Created attachment 354917 [details]
Patch for landing
Comment 6 Thibault Saunier 2018-11-15 05:12:43 PST
Created attachment 354918 [details]
Patch for landing
Comment 7 Thibault Saunier 2018-11-15 05:13:13 PST
(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
Comment 8 WebKit Commit Bot 2018-11-15 05:15:18 PST
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 commit-queue@webkit.org...
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
Comment 9 Xabier Rodríguez Calvar 2018-11-15 05:50:48 PST
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
Comment 10 Thibault Saunier 2018-11-15 06:01:26 PST
Created attachment 354924 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2018-11-15 06:39:36 PST
Comment on attachment 354924 [details]
Patch for landing

Clearing flags on attachment: 354924

Committed r238224: <https://trac.webkit.org/changeset/238224>
Comment 12 WebKit Commit Bot 2018-11-15 06:39:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-11-15 06:40:24 PST
<rdar://problem/46094436>