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 limitationOkay 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.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
