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 709408 - audioconvert: modifies buffer mapped for READ
audioconvert: modifies buffer mapped for READ
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal normal
: 1.2.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-10-04 05:07 UTC by Matej Knopp
Modified: 2013-10-10 10:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audioconvert: map memory as readwrite if buffer is writable (955 bytes, patch)
2013-10-04 05:07 UTC, Matej Knopp
needs-work Details | Review
Path to check if buffer and memory is writable (1.79 KB, patch)
2013-10-04 11:59 UTC, Matej Knopp
committed Details | Review

Description Matej Knopp 2013-10-04 05:07:35 UTC
Created attachment 256443 [details] [review]
audioconvert: map memory as readwrite if buffer is writable

the input buffer is always mapped as READ, even if the data gets modified. This causes problem when the memory is shared between multiple buffers.

(patch might need rebasing, let me know if it does).
Comment 1 Edward Hervey 2013-10-04 06:39:50 UTC
looks good in principle.

It's interesting, I thought there was a rationale/rule that you couldn't write in the input buffer provided by basetransform when not working in-place... but I couldn't find anything in basetransform's code.
Comment 2 Sebastian Dröge (slomo) 2013-10-04 09:03:57 UTC
I think it's a bit unexpected that this buffer is modified though. Where does this happen in the audioconvert code? And you only map writable if the buffer is writable, but I assume the data of that buffer is also modified in other situations?
Comment 3 Matej Knopp 2013-10-04 11:04:05 UTC
gboolean
audio_convert_convert (AudioConvertCtx * ctx, gpointer src,
    gpointer dst, gint samples, gboolean src_writable)

later src is used as tmpbuf

if ((insize >= biggest) && src_writable && (ctx->in.bpf >= size))
    tmpbuf = src;

it looks like just a way to avoid creating temporary buffer. 

Thinking of it, perhaps src_writable should only be set if buffer is writable and the memory is writable, otherwise it just copies the entire memory instead of creating temporary buffer.
Comment 4 Sebastian Dröge (slomo) 2013-10-04 11:19:20 UTC
Yes, that sounds sensible
Comment 5 Matej Knopp 2013-10-04 11:59:29 UTC
Created attachment 256467 [details] [review]
Path to check if buffer and memory is writable
Comment 6 Sebastian Dröge (slomo) 2013-10-04 12:02:29 UTC
commit 12f85c325db81e36d12bcbb348d4806eb5057458
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Fri Oct 4 13:57:51 2013 +0200

    audioconvert: Map buffer as READWRITE if the buffer and memory is writable
    
    and only use the input buffer as temporary buffer in that case.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709408