After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 776983 - opusenc does not discard silence if DTX enabled
opusenc does not discard silence if DTX enabled
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.10.x
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-01-07 15:05 UTC by Nubosch
Modified: 2018-11-03 11:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
discard frames marked as dtx (821 bytes, patch)
2017-01-07 15:07 UTC, Nubosch
none Details | Review
v02 discard frames marked as dtx (918 bytes, patch)
2017-01-08 11:23 UTC, Nubosch
none Details | Review
v03 discard frames marked as dtx (866 bytes, patch)
2017-01-08 11:34 UTC, Nubosch
none Details | Review
opusenc: discard frames marked as dtx v04 (969 bytes, patch)
2017-01-16 12:33 UTC, Nubosch
none Details | Review
gstrtpopuspay: discard frames marked as dtx (461 bytes, patch)
2017-01-16 12:34 UTC, Nubosch
none Details | Review
gstrtpopuspay: discard frames marked as dtx (500 bytes, patch)
2017-01-17 11:37 UTC, Nubosch
none Details | Review

Description Nubosch 2017-01-07 15:05:35 UTC
if dtx is enabled and silence is detected opusenc should only encode 1 frame each 400ms, as stated in RFC 6716, 2.1.9.

in function gst_opus_enc_encode() 1 is returned from opus_multistream_encode(), signaling the frame should be discarded cause of detected silence.
Each 400ms a value != 1 is returned from opus_multistream_encode().

I use the attached patch to discard the frame.
can you tell me why this might be a bad idea?

I use this pipeline for testing
gst-launch-1.0 pulsesrc ! "audio/x-raw,channels=1,rate=48000" ! opusenc audio-type=2048 bandwidth=1103 bitrate=20000 bitrate-type=1 inband-fec=TRUE packet-loss-percentage=1 dtx=TRUE ! fakesink

I came across this while measuring network bandwidth using opusenc with dtx and rtpopuspay.
The packet rate was constant, one packet each 20ms.
There where packets containing only one single RTP payload byte.
With the attached patch the packet rate is irregular with a minimum of one packet each 400ms, always containing multiple bytes.
Comment 1 Nubosch 2017-01-07 15:07:22 UTC
Created attachment 343091 [details] [review]
discard frames marked as dtx
Comment 2 Nubosch 2017-01-08 11:21:27 UTC
(In reply to Nubosch from comment #0)
> if dtx is enabled and silence is detected opusenc should only encode 1 frame
> each 400ms, as stated in RFC 6716, 2.1.9.
> 
> in function gst_opus_enc_encode() 1 is returned from
> opus_multistream_encode(), signaling the frame should be discarded cause of
> detected silence.
> Each 400ms a value != 1 is returned from opus_multistream_encode().
> 
> I use the attached patch to discard the frame.
> can you tell me why this might be a bad idea?

gathering enough information is not as easy as I thought.
there was some discussion touching this topic here
https://patches.libav.org/patch/27718/

in fact the non multi stream api tells:
"If the return value is 2 bytes or less, then the packet does not need to be transmitted (DTX)"

I was not able to find a reasonable information about the multi stream api.
so I played around with the amount of channels and encoding parameters.

It looks like, if opus is able to actually perform dtx it adds a TOC and a 0 length octet, skipping the length octet for the last stream.

- is checking for dtx property enough?
- does it make sense to check for
  outsize > (2*(enc->n_channels - enc->n_stereo_streams))

imo >= should not be needed, as adding another length octet would require a payload to exists.

> I use this pipeline for testing
> gst-launch-1.0 pulsesrc ! "audio/x-raw,channels=1,rate=48000" ! opusenc
> audio-type=2048 bandwidth=1103 bitrate=20000 bitrate-type=1 inband-fec=TRUE
> packet-loss-percentage=1 dtx=TRUE ! fakesink
> 
> I came across this while measuring network bandwidth using opusenc with dtx
> and rtpopuspay.
> The packet rate was constant, one packet each 20ms.
> There where packets containing only one single RTP payload byte.
> With the attached patch the packet rate is irregular with a minimum of one
> packet each 400ms, always containing multiple bytes.

RFC 7587 does not include multiple streams as also rtpopuspay does not include it
Comment 3 Nubosch 2017-01-08 11:23:33 UTC
Created attachment 343112 [details] [review]
v02 discard frames marked as dtx
Comment 4 Nubosch 2017-01-08 11:34:51 UTC
Created attachment 343113 [details] [review]
v03 discard frames marked as dtx

remove debug output,
remove experiment setting outsize 0, use -1
sorry for the noise :/
Comment 5 Olivier Crête 2017-01-11 18:17:15 UTC
Review of attachment 343113 [details] [review]:

You should push a GAP event instead of a buffer. It seems that the base class has nothing to help, so something like ret = gst_pad_push (encoder->srcpad, timestamp, duration); should be enough if the finish frame didnt return an error.
Comment 6 Nubosch 2017-01-13 13:23:15 UTC
many thanks for your review.

the buffer wrapped around the data in the adapter does not carry the time information. Should this be generated by gst_audio_encoder_push_buffers?

Maybe it is a good idea to go the way speexenc did?
Just marking the buffer with GST_BUFFER_FLAG_GAP, and dropping it in rtpopuspay?

Another option would be to add the event to gst_audio_encoder_finish_frame where all required information seems to be available, but I'm unsure about this. 

Sorry for bothering you with another question.
Do you think the dtx-drop feature should be optional?
Without a property this may break existing implementations.

The comfort noise generation in opus is not triggered,
also the jitterbuffer goes crazy, maybe dropping valid audio data.
This is the same situation for speex, but it may be acceptable depending on the use-case.

having a dtx aware jitterbuffer would be cool, but this is another story, and much more complicated than this simple changes...
Comment 7 Olivier Crête 2017-01-13 18:55:13 UTC
Marking it as FLAG_GAP, and then dropping it either in the payloader or pre_push() both seem fine. The question is how muxing to Ogg (or other containers) is affected by dropping DTX frames.

Comfort noise generation should be triggered when the jitterbuffer emits an event because an expected buffer is missing.

I'm not sure what a drop-dtx property would do more than just turning off DTX in practice, for Opus, the codec compresses enough that I would expect that the overhead of sending "empty" dtx packets would make you lose half of the benefit anyway.

If the jitterbuffer goes crazy in presence of DTX, this is definitely something that should be fixed in the JB. DTX is a real world feature we should support.
Comment 8 Nubosch 2017-01-16 12:31:36 UTC
(In reply to Olivier Crête from comment #7)
> Marking it as FLAG_GAP, and then dropping it either in the payloader or
> pre_push() both seem fine. The question is how muxing to Ogg (or other
> containers) is affected by dropping DTX frames.

I decided to drop in the payloader, so everything on a local machine is not affected by this change. I expect the issues in a muxer will be the same as for e.g. pulsesink "discontinuity in audio"

> Comfort noise generation should be triggered when the jitterbuffer emits an
> event because an expected buffer is missing.

see below

> I'm not sure what a drop-dtx property would do more than just turning off
> DTX in practice, for Opus, the codec compresses enough that I would expect
> that the overhead of sending "empty" dtx packets would make you lose half of
> the benefit anyway.

100% ACK.
 
> If the jitterbuffer goes crazy in presence of DTX, this is definitely
> something that should be fixed in the JB. DTX is a real world feature we
> should support.

I'm in doubt I won't have the time to look into this in the near future.
At least I opened a new bug report documenting what I know so far.

https://bugzilla.gnome.org/show_bug.cgi?id=777319

I really hope I can come back to this after solving my 1001 network issues not related to gstreamer.

many thanks for your help!
Comment 9 Nubosch 2017-01-16 12:33:24 UTC
Created attachment 343547 [details] [review]
opusenc: discard frames marked as dtx v04

instead of dropping the frame mark it as gap, so it can be dropped in the payloader
Comment 10 Nubosch 2017-01-16 12:34:30 UTC
Created attachment 343548 [details] [review]
gstrtpopuspay: discard frames marked as dtx
Comment 11 Nubosch 2017-01-17 11:37:39 UTC
Created attachment 343635 [details] [review]
gstrtpopuspay: discard frames marked as dtx

do I really have to do all the silly mistakes possible?

v01 fix memory leak
Comment 12 GStreamer system administrator 2018-11-03 11:53:23 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/324.