[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