[FFmpeg-devel] [PATCH 6/6] lavc/flacdsp: document limitations of the LPC encoder
James Darnley
james.darnley at gmail.com
Wed Aug 13 14:30:20 CEST 2014
On 2014-08-13 14:05, Michael Niedermayer wrote:
> On Wed, Aug 13, 2014 at 10:46:45AM +0200, James Darnley wrote:
>> On 2014-08-13 04:50, Michael Niedermayer wrote:
>>> On Tue, Aug 12, 2014 at 11:22:07PM +0200, James Darnley wrote:
>>>> ---
>>>> libavcodec/flacdsp.h | 7 +++++++
>>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h
>>>> index 272cf2a..36cd904 100644
>>>> --- a/libavcodec/flacdsp.h
>>>> +++ b/libavcodec/flacdsp.h
>>>> @@ -27,6 +27,13 @@ typedef struct FLACDSPContext {
>>>> int len, int shift);
>>>> void (*lpc)(int32_t *samples, const int coeffs[32], int order,
>>>> int qlevel, int len);
>>>> + /**
>>>> + * This function has some limitations with various configurations:
>>>> + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the
>>>> + * maximum value of order is 32.
>>>> + * - when SSE4 (or newer) is available on x86 there is an unrolled copy
>>>> + * which also assumes the maximum value of order is 0.
>>>> + */
>>>
>>> sounds like
>>>
>>> printf()
>>> on fridays with SSE4 this is limited to 27 characters
>>>
>>> a function either should have a limit or not have one, it should
>>> not depend on other factors
>>>
>>> People using this function must be able to tell in what cases they
>>> can use it
>>>
>>> and People optimizing the function need to know which cases their
>>> optimized code must support
>>>
>>> the API defines both
>>
>> I don't get it. FLAC only allows upto 32-order LPC. This was,
>> apprarently, an acceptable assumption to make for the unrolled C code
>> yet somehow I can't make the same assumption for assembly.
>
> theres some kind of misunderatanding here
>
> its perfectly fine to say
>
> /**
> * This function supports upto a order of 32
> */
>
> what i think is a really bad idea is to make the API conditional
> on cpu features and build flags.
> What if someone wants to add a SSE2 optimization that works just up
> to order 32 ? he cannot do it without changing the API and checking
> that all uses of the API are safe with the new limitation
Okay I understand that.
I thought that if someone wanted to re-use the function in some other
encoder which might allow 64 order (for example), I should document
where the limitations are.
I can change the patch to state that it supports up to 32 but should I
also still mention where that is assumed?
What about Christophe's suggestion of changing the function definition
by using "int coeffs[32]"?
Personally I am in favour of something more verbose that just stating
one limit.
>>>> + * which also assumes the maximum value of order is 0.
>> ^^^
>> Is it this bit that is confusing? Because I only now see that typo. Of
>> course that should say "32".
>
> that too
Sorry about that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 618 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140813/db605f51/attachment.asc>
More information about the ffmpeg-devel
mailing list