GNOME Bugzilla – Bug 773833
rtpL16depay, rtpL24depay: support depayloading in place
Last modified: 2018-11-03 15:13:58 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.
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?
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.
(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)
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...
(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).
(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.
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.
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
(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.
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?
Created attachment 339374 [details] [review] Do read-write buffer mapping only if the channel positions differ. Here is the corrected patch.
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
-- 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.