Created attachment 302398 [details] [review]
spandsp: Don't use the private field 'missing_samples'

Attachment 302398 [details] pushed as 5b22a12 - spandsp: Don't use the private field 'missing_samples'

Review of attachment 302398 [details] [review]:

.

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>.

The thing is that next like unconditionally maps read/write the buffer. Maybe there is more issues ?

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.

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.

You need a writable buffer before you can do gst_buffer_map(...WRITE);

I'm not quite sure I understand what the remaining issues are here? Nicolas?

(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.

LRN, so why is this needed?

Is it an optimisation? (So we don't map it in WRITE mode if READ would be sufficient?)

I've asked on spandsp ML[1], no answer.

AFAICS, it's an optimization though.

[1] http://lists.soft-switch.org/pipermail/spandsp/2015-April/000202.html

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 ?

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).

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).