[Ffmpeg-devel] [PATCH] avcodec_open Doxygen docs

[Panagiotis Issaris]

Hi,

Michael Niedermayer schreef:
> On Thu, Feb 22, 2007 at 12:50:52AM +0100, Panagiotis Issaris wrote:
> [...]
>>>
>>> there is no gurantee that future lavc implementations might not return
>>> a richer set of error codes or success codes
>>>  
>>>       
>> Fixed.
>> In fact, I'd like to start adding such error codes. Where should they 
>> go? avcodec.h or some separate file
>> like av_error.h?
>>     
>
> see avcodec.h AVERROR*
>   
Thanks.

>>> [...]
>>>       
>> Size of the output buffer?
>>     
>>> and the 32/64 input padding applies to audio too!
>>>  
>>>       
>> Fixed.
>>
>> Does the output and input buffer also have to be aligned on a 16 byte 
>> boundary for avcodec_decode_video()?
>>     
>
> the true alignment requirements depend for every buffer on the cpu that
> is if the cpu has no alignment requirements then it all might work fine
> with no alignment at all though maybe slower (that again depends on the cpu)
>
> in practice though the bitstream should have 4byte alignment at minimum and
> all sample data be it audio or video should be 16byte aligned unless the
> cpu doesnt need it (altivec/sse do) that also means if the linesize is not
> a multiple of 16 then theres no sense in aligning the buffer start to 16
>   
I've added a note to both avcodec_decode_video() and 
avcodec_decode_audio() saying:
"You might have to align the input buffer \p buf and output buffer \p
samples. The alignment requirements depend on the CPU: on some CPUs it isn't
necessary at all, on others it won't work at all if not aligned and on 
others
it will work but it will have an impact on performance. In practice, the
bitstream should have 4 byte alignment at minimum and all sample data should
be 16 byte aligned unless the CPU doesn't need it (AltiVec and SSE do). If
the linesize is not a multiple of 16 then there's no sense in aligning the
start of the buffer to 16."

Does that sound alright?

> [...]
>   
>> @@ -2493,24 +2493,93 @@ extern AVCodec *first_avcodec;
>>  unsigned avcodec_version(void);
>>  /* returns LIBAVCODEC_BUILD constant */
>>  unsigned avcodec_build(void);
>> +
>> +/**
>> + * Initializes libavcodec.
>> + *
>> + * @warning This function \e must be called before any other functions.
>> + */
>>  void avcodec_init(void);
>>     
>
> this is not good if taken literally, it should rather be any other libavcodec
> function or something similar
>   
Fixed.
>
>   
>> +/**
>> + * Find a decoder with a matching codec ID.
>>     
>
> my engrish is shitty but something says that this should be "findS"
>   
:-D
Well, ... I surely was inconsistent -not on purpose though- as I wrote 
"Initializes libavcodec."
followed by the "Find ..." comments...  :)
Fixed.

>
>   
>> + *
>> + * @param id CodecID of the requested decoder.
>> + * @return An decoder if one was found, NULL otherwise.
>>     
>
> and here my feeling says "a decoder"
>   
Indeed, I had copy & pasted it from "An encoder" :)
Fixed.
> [...]
>   
>> +/**
>> + * Sets its fields of the given AVFrame to default values.
>>     
>
> s/its/the/
>   
Fixed.
> [...]
>   
>> +/**
>> + * Checks if the given dimension of a picture is valid.
>> + *
>>     
>
> at this point noone will have learnt what the function does, what is "valid"
>
> because 80000x80000 is valid in some sense ...
>   
Hmm. You are right...

Would something like this be better? Or should I explicitly mention the 
checks?

2578  * Checks if the given dimension of a picture is valid, meaning 
that width and
2579  * height are sane and the total picture size is allocatable.

I'm not sure actually about the (w+128)*(uint64_t)(h+128) < INT_MAX/4) 
part of the
check: For 64-bit machines, couldn't this check be too strict? IOW isn't 
INT_MAX
still somewhere near 2^31 on 64-bit machines?

or:
2578  * Checks if the given dimension of a picture is valid, meaning 
that width and
2579  * height are sane and the total picture size is addressable.

or:
2578  * Checks if the given dimension of a picture is valid, meaning 
that width and
2579  * height and total picture size are sane.

And preference? Or a better suggestion maybe?

>
> [...]
>   
>>                           uint8_t *buf, int buf_size);
>> +
>>  /**
>>     
>
> cosmetic :)
>   
Oops :)

>
>   
>> - * Decode an audio frame.
>> + * Decodes an audio frame from \p buf into \p samples.
>> + * The avcodec_decode_audio2() function decodes a frame of audio from the input
>> + * buffer \p buf of size \p buf_size. To decode it, it makes use of the
>> + * audiocodec which was coupled with \p avctx using avcodec_open(). The
>> + * resulting decoded frame is stored in output buffer \p samples.  If no frame
>> + * could be decompressed, \p frame_size_ptr is zero. Otherwise, it is the
>> + * decompressed frame size in \e bytes.
>> + *
>> + * @warning You \e must set \p frame_size_ptr to the allocated size of the
>> + * output buffer before calling avcodec_decode_audio2().
>> + *
>> + * @warning The input buffer must be \c FF_INPUT_BUFFER_PADDING_SIZE larger than
>> + * the actual read bytes because some optimized bitstream readers read 32 or 64
>> + * bits at once and could read over the end.
>>   *
>> - * @param avctx the codec context.
>> - * @param samples output buffer, 16 byte aligned
>> - * @param frame_size_ptr the output buffer size in bytes (you MUST set this to the allocated size before calling avcodec_decode_audio2()), zero if no frame could be compressed
>> - * @param buf input buffer, 16 byte aligned
>> - * @param buf_size the input buffer size
>> - * @return 0 if successful, -1 if not.
>>     
>
>
>   
>> + * @warning The end of the input buffer \p buf should be set to 0 to ensure that
>> + * no overreading happens for damaged MPEG streams.
>>     
>
> this trick is limited to video i _think_ though of course it does no harm
> for audio
>   
Ah, apiexample.c used it for audio decoding too, which is why I added 
it. Attached patch removes it in case anyone
confirms to be 100% sure it does no harm to remove it.

 apiexample.c |    3 ---
 1 file changed, 3 deletions(-)

Thanks for reviewing!

I haven't added an updated patch until I can fix the 
avcodec_check_dimensions and alignment related
comments based on your reply.

With friendly regards,
Takis
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pi-20070222T024933-ffmpeg-unnecessary_memset.diff
Type: text/x-patch
Size: 520 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070222/80321b73/attachment.bin>