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 748497 - spandsp: Fails to build, uses a private field missing_samples
spandsp: Fails to build, uses a private field missing_samples
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2015-04-26 21:07 UTC by LRN
Modified: 2015-06-14 15:48 UTC
See Also:
GNOME target: ---
GNOME version: ---

spandsp: Don't use the private field 'missing_samples' (1.06 KB, patch)
2015-04-26 21:07 UTC, LRN
none Details | Review
spandsp: Include private headers to access 'missing_samples' (804 bytes, patch)
2015-04-27 14:35 UTC, LRN
none Details | Review

Description LRN 2015-04-26 21:07:02 UTC
missing_samples is a private field, can't be used.
The version of spandsp for which this is true is at least 0.0.6 20140809 snapshot.
Comment 1 LRN 2015-04-26 21:07:08 UTC
Created attachment 302398 [details] [review]
spandsp: Don't use the private field 'missing_samples'
Comment 2 Nicolas Dufresne (ndufresne) 2015-04-27 14:29:53 UTC
Attachment 302398 [details] pushed as 5b22a12 - spandsp: Don't use the private field 'missing_samples'
Comment 3 Nicolas Dufresne (ndufresne) 2015-04-27 14:34:31 UTC
Review of attachment 302398 [details] [review]:

Comment 4 LRN 2015-04-27 14:35:07 UTC
Created attachment 302450 [details] [review]
spandsp: Include private headers to access 'missing_samples'

Try a different approach. Since the structure in question is still described in the headers, albeit in private ones, we can still use it for performance reasons. README in spandsp/private/ explicitly mentions that as valid use-case.
This should compile as long as you have <spendsp/expose.h>.
Comment 5 Nicolas Dufresne (ndufresne) 2015-04-27 14:51:11 UTC
The thing is that next like unconditionally maps read/write the buffer. Maybe there is more issues ?
Comment 6 LRN 2015-04-27 15:02:32 UTC
From what i can see, plc_rx() is guaranteed to at least read the buffer, and might write it if plc->plc_state->missing_samples != 0.

At best we can make gst code maps it as read-only if plc->plc_state->missing_samples == 0 and readwrite if plc->plc_state->missing_samples != 0, and hope that my analysis of plc_rx() behaviour is correct and that it only writes the buffer when plc->plc_state->missing_samples != 0, otherwise we're screwed.
Comment 7 Nicolas Dufresne (ndufresne) 2015-04-27 15:25:10 UTC
Also note that gst_buffer_make_writable() only makes the GstBuffer writable, read-only memory will remain read-only. Strictly speaking, make_writable() isn't really needed to map a buffer (I didn't check if we change anything else later in the code). Not checking the return value of buffer_map() here is quite terrible also.
Comment 8 Olivier Crête 2015-04-27 15:42:48 UTC
You need a writable buffer before you can do gst_buffer_map(...WRITE);
Comment 9 Tim-Philipp Müller 2015-06-02 23:22:32 UTC
I'm not quite sure I understand what the remaining issues are here? Nicolas?
Comment 10 Nicolas Dufresne (ndufresne) 2015-06-03 00:41:38 UTC
(In reply to Tim-Philipp Müller from comment #9)
> I'm not quite sure I understand what the remaining issues are here? Nicolas?

Reviewing the new patch is what is left to do. The new patch include the private header rather then try and stop using the private member. I don't know much about this member, or why we need it or not.
Comment 11 Tim-Philipp Müller 2015-06-03 11:10:53 UTC
LRN, so why is this needed?

Is it an optimisation? (So we don't map it in WRITE mode if READ would be sufficient?)
Comment 12 LRN 2015-06-03 15:53:30 UTC
I've asked on spandsp ML[1], no answer.

AFAICS, it's an optimization though.

Comment 13 Nicolas Dufresne (ndufresne) 2015-06-03 16:19:29 UTC
It does not seem to risky, it's not like they rewriting these thing very often. I'd give my +1 to this patch. Any one else ?
Comment 14 Tim-Philipp Müller 2015-06-03 18:15:45 UTC
I don't really mind either way.

I wonder if this isn't premature optimisation though.

The main question is if we are really guaranteed that it won't write to the memory otherwise (now or in future).
Comment 15 Tim-Philipp Müller 2015-06-14 15:48:13 UTC
Let's close this. I think the current code is good as it is, and the risk/benefit ratio of changing it as suggested seems high.

Seeing that accessing that private member is only an optimisation *and* we don't even know for sure if it's a legit optimisation/assumption to make, we should probably just leave it as is. Besides, the performance impact of gst_buffer_make_writable() is tiny, and skipping it is wrong anyway if we map with READWRITE (what we should've done instead is to map as READ only I guess).