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 773833 - rtpL16depay, rtpL24depay: support depayloading in place
rtpL16depay, rtpL24depay: support depayloading in place
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-02 14:43 UTC by Petr Kulhavy
Modified: 2018-11-03 15:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Do read-write buffer mapping only if the channel positions differ. (2.63 KB, patch)
2016-11-07 11:15 UTC, Petr Kulhavy
none Details | Review
Do read-write buffer mapping only if the channel positions differ. (2.40 KB, patch)
2016-11-09 10:26 UTC, Petr Kulhavy
committed Details | Review

Description Petr Kulhavy 2016-11-02 14:43:43 UTC
It seems that rtpL16depay, rtpL24depay (and I guess also other rtp-depay functions) are failing to depayload in place. It always allocates a new buffer and copies the payload.

This is the log I'm seeing:

0:00:13.489419481  1060  0x1ec5150 DEBUG            rtpL16depay gstrtpL16depay.c:243:gst_rtp_L16_depay_process:<rtpl16depay0> got payload of 192 bytes
0:00:13.489596481  1060  0x1ec5150 LOG               GST_BUFFER gstbuffer.c:798:gst_buffer_new: new 0x75c05370
0:00:13.489760981  1060  0x1ec5150 LOG               GST_BUFFER gstbuffer.c:2038:gst_buffer_copy_region: new region copy 0x75c05370 of 0x75c054b0 12-192
0:00:13.490969772  1060  0x1ec5150 LOG               GST_BUFFER gstbuffer.c:514:gst_buffer_copy_into: copy 0x75c054b0 to 0x75c05370, offset 12-192/204
--
0:00:13.493352979  1060  0x1ec5150 TRACE            GST_LOCKING gstminiobject.c:177:gst_mini_object_lock: lock 0x1ecb690: state 00020000, access_mode 3
0:00:13.493546312  1060  0x1ec5150 DEBUG            GST_LOCKING gstminiobject.c:212:gst_mini_object_lock: lock failed 0x1ecb690: state 00020000, access_mode 3
0:00:13.493717020  1060  0x1ec5150 DEBUG             GST_MEMORY gstmemory.c:317:gst_memory_map: mem 0x1ecb690: lock 3 failed
0:00:13.493910270  1060  0x1ec5150 TRACE        GST_REFCOUNTING gstobject.c:249:gst_object_ref:<allocatorsysmem0> 0x1ddc010 ref 6->7
0:00:13.494091270  1060  0x1ec5150 DEBUG             GST_MEMORY gstmemory.c:138:gst_memory_init: new memory 0x1ecb578, maxsize:195 offset:0 size:192
0:00:13.494364978  1060  0x1ec5150 DEBUG        GST_PERFORMANCE gstallocator.c:462:_sysmem_copy: memcpy 192 memory 0x1ecb690 -> 0x1ecb578


And this is the command:
GST_DEBUG=9 gst-launch-1.0 audiotestsrc is-live=true wave=silence samplesperbuffer=48 ! "audio/x-raw,format=(string)S16BE,rate=(int)48000,channels=(int)2,channel-mask=(int)0x3" ! rtpL16pay ! rtpL16depay ! fakesink
2>&1 | grep -B 5 copy

It happens in gst_audio_buffer_reorder_channels() when mapping the buffer read-write. It doesn't really matter what the source is, the same happens with udpsrc.

--

The other thing here is that if the channels don't require reordering (like in my case) the buffer shouldn't be remapped read-write. Otherwise the potential buffer copy is redundant.
Comment 1 Sebastian Dröge (slomo) 2016-11-04 14:36:21 UTC
The problem is here: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtp/gstrtpL16depay.c?id=752dd15c54238c389f676105867ff7815d623ced#n245

The payload buffer is retrieved and tried to be modified while the RTP buffer is still referenced (the caller owns the reference). As such, modification first needs to copy the whole payload. Fixing this will require API changes/additions in GstRTPBaseDepayload.


For the other part, that should be easily solveable by checking (once when the channel-layout is known) if any reordering has to happen, remember that and just not do it if not needed.

Do you want to provide a patch for the second part?
Comment 2 Petr Kulhavy 2016-11-05 12:09:53 UTC
Regarding the second part gst_audio_buffer_reorder_channels() already performs the check whether any reordering is needed. So it should be either removed from there or the gst_buffer_make_writable() moved into gst_audio_buffer_reorder_channels(). It makes no sense that the check is done twice. 

My preferred way would be to move gst_buffer_make_writable() into gst_audio_buffer_reorder_channels() because the same problem is also in gstrtpL16pay, gstrtpL24pay and gstrtpL24depay. However I don't know what APIs would be affected by that...

If none of the above is acceptable I would suggest at least to extend the audio-channels API with a function, which performs the check if reordering is needed.
Comment 3 Sebastian Dröge (slomo) 2016-11-05 12:14:51 UTC
(In reply to Petr Kulhavy from comment #2)
> Regarding the second part gst_audio_buffer_reorder_channels() already
> performs the check whether any reordering is needed. So it should be either
> removed from there or the gst_buffer_make_writable() moved into
> gst_audio_buffer_reorder_channels(). It makes no sense that the check is
> done twice. 

Could be removed I guess, inside the function. My idea was to do the check exactly once when the caps are set and never call that function at all then, instead of having the function do the check every time when no reordering is performed.

> My preferred way would be to move gst_buffer_make_writable() into
> gst_audio_buffer_reorder_channels() because the same problem is also in
> gstrtpL16pay, gstrtpL24pay and gstrtpL24depay. However I don't know what
> APIs would be affected by that...
> 
> If none of the above is acceptable I would suggest at least to extend the
> audio-channels API with a function, which performs the check if reordering
> is needed.

That can't be moved into the function as is, we would need to add a new function that a) takes ownership of the buffer passed to it, and b) returns a new buffer (or the unchanged input buffer).

Such a function would indeed make sense, but would also not solve the problem here. It's just nicer :)

We should ideally never ever call this function if not needed (and the first problem is completely independent of all we talked about here so far anyway)
Comment 4 Petr Kulhavy 2016-11-07 09:58:16 UTC
I've starting writing the patch for the second part and I'm wondering if the gst_rtp_drop_meta() in gst_rtp_L16_depay_process() also requires gst_buffer_make_writable() to be called beforehand?

Because if yes then the copy is inevitable even in case the channels come already in the right order...
Comment 5 Sebastian Dröge (slomo) 2016-11-07 10:09:22 UTC
(In reply to Petr Kulhavy from comment #4)
> I've starting writing the patch for the second part and I'm wondering if the
> gst_rtp_drop_meta() in gst_rtp_L16_depay_process() also requires
> gst_buffer_make_writable() to be called beforehand?
> 
> Because if yes then the copy is inevitable even in case the channels come
> already in the right order...

We could make it so that it only requires writability if there are metas to be dropped. But your problem is not the make_writable() AFAIU, but that you need to map the buffer writable. These are two different aspects. The writability of the buffer (the metadata) and the memory (the actual media).
Comment 6 Petr Kulhavy 2016-11-07 10:52:21 UTC
(In reply to Sebastian Dröge (slomo) from comment #5)
> We could make it so that it only requires writability if there are metas to
> be dropped. But your problem is not the make_writable() AFAIU, but that you
> need to map the buffer writable. These are two different aspects. The
> writability of the buffer (the metadata) and the memory (the actual media).

Ah, sorry, I've mixed up two different things. Now it makes more sense.
Comment 7 Petr Kulhavy 2016-11-07 11:15:47 UTC
Created attachment 339238 [details] [review]
Do read-write buffer mapping only if the channel positions differ.

So here is the patch for the easy part.
Comment 8 Sebastian Dröge (slomo) 2016-11-08 12:38:14 UTC
Review of attachment 339238 [details] [review]:

Generally looks good, thanks :) Just some minor comments below (also don't put this "Signed-off-by" thing into the commit message, we're not doing that)

::: gst-libs/gst/audio/audio-channels.c
@@ +131,3 @@
+ * In other words, tells whether channel reordering is needed (unequal) or not (equal).
+ *
+ * Returns: %TRUE if the channel positions are equal, i.e. no reordering is needed.

Add a "Since: 1.12" here

Also needs to be added to the .def file in win32/common and docs/libs/*sections.txt at the right place
Comment 9 Petr Kulhavy 2016-11-08 18:42:15 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> Review of attachment 339238 [details] [review] [review]:
> 
> Generally looks good, thanks :) Just some minor comments below (also don't
> put this "Signed-off-by" thing into the commit message, we're not doing that)

Ah, ok. That's my habit from Linux kernel to use "git commit -s" for commits :-)

> ::: gst-libs/gst/audio/audio-channels.c
> @@ +131,3 @@
> + * In other words, tells whether channel reordering is needed (unequal) or
> not (equal).
> + *
> + * Returns: %TRUE if the channel positions are equal, i.e. no reordering is
> needed.
> 
> Add a "Since: 1.12" here
> 
> Also needs to be added to the .def file in win32/common and
> docs/libs/*sections.txt at the right place

Please note that despite the description the function is private (static). Originally I thought to make it public but then I realized that it's used in only in the same C file. So I made it static but left the comment there.
Comment 10 Sebastian Dröge (slomo) 2016-11-09 07:17:49 UTC
Ah indeed. Please don't make it a gtk-doc comment then (i.e. don't have the double ** at the beginning) :) Can you update the patch?
Comment 11 Petr Kulhavy 2016-11-09 10:26:44 UTC
Created attachment 339374 [details] [review]
Do read-write buffer mapping only if the channel positions differ.

Here is the corrected patch.
Comment 12 Sebastian Dröge (slomo) 2016-11-09 17:43:27 UTC
Comment on attachment 339374 [details] [review]
Do read-write buffer mapping only if the channel positions differ.

commit 54f4d3772c906090870c6a79b6bb7940c105e1f1
Author: Petr Kulhavy <brain@jikos.cz>
Date:   Mon Nov 7 12:01:16 2016 +0100

    audio-channels: map buffer read-write only if channels differ
    
    gst_audio_buffer_reorder_channels() was always mapping the buffer read-write
    regardless whether any reordering was needed.  If the from and to channel order
    is identical return immediately without remapping the buffer.
    
    Add a small helper function gst_audio_channel_positions_equal() which is used
    in both gst_audio_reorder_channels() and gst_audio_buffer_reorder_channels().
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773833
Comment 13 GStreamer system administrator 2018-11-03 15:13:58 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-good/issues/320.