[FFmpeg-devel] [PATCH] extract bit rate calculation into separate function

[Robert Krüger]

Hi,

On 12.11.2009, at 01:42, Stefano Sabatini wrote:

> On date Wednesday 2009-11-11 12:41:33 +0100, Robert Kr?ger encoded:
> [...]
>> Index: libavcodec/avcodec.h
>> ===================================================================
>> --- libavcodec/avcodec.h	(revision 20511)
>> +++ libavcodec/avcodec.h	(working copy)
>> @@ -3444,6 +3444,14 @@
>>  */
>> int av_get_bits_per_sample_format(enum SampleFormat sample_fmt);
>>
>> +/**
>> + * Calculates the bit rate of a stream
>
> Nit, ending "." missing.
>
ok, will fix

>> + *
>> + * @ctx AVCodecContext of the stream
>
> this is:
> @param ctx ...
>
will fix

>> + * @return Bit rate in bits per second or 0 if it could not be
>> determined
>
> *b*it rate, lowcased is preferred here.
>
all other functions I looked at in the same header, had this kind of  
capitalization on the return statement, however it does not seem to be  
consistent as for some functions param description start with lower  
case and return value descriptions with upper case. I'll change it as  
you say.

>> + */
>> +int av_get_bitrate(AVCodecContext *ctx);
>> +
>> /* frame parsing */
>> typedef struct AVCodecParserContext {
>>     void *priv_data;
>> Index: libavcodec/utils.c
>> ===================================================================
>> --- libavcodec/utils.c	(revision 20511)
>> +++ libavcodec/utils.c	(working copy)
>> @@ -741,6 +741,34 @@
>>     return NULL;
>> }
>>
>> +int av_get_bitrate(AVCodecContext *ctx)
>> +{
>> +    int bitrate;
>
> I slightly prefer bit_rate, as more consistent with ctx->bit_rate.
>
OK, will change

>> +    int bits_per_sample;
>> +
>> +    switch(ctx->codec_type) {
>> +    case CODEC_TYPE_VIDEO:
>> +  	    bitrate = ctx->bit_rate;
>> +        break;
>> +    case CODEC_TYPE_AUDIO:
>> +    	bits_per_sample = av_get_bits_per_sample(ctx->codec_id);
>> +    	bitrate = bits_per_sample ? ctx->sample_rate * ctx->channels *
>> bits_per_sample : ctx->bit_rate;
>> +        break;
>
>> +    case CODEC_TYPE_DATA:
>> +        bitrate = ctx->bit_rate;
>> +        break;
>> +    case CODEC_TYPE_SUBTITLE:
>> +        bitrate = ctx->bit_rate;
>> +        break;
>> +    case CODEC_TYPE_ATTACHMENT:
>> +        bitrate = ctx->bit_rate;
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +    return bitrate;
>> +}
>> +
>
> VIDEO, DATA, SUBTITLE, ATTACHMENT and default cases can be factorized.
>
> Maybe it's simpler:
> if (type == CODEC_AUDIO && (bits_per_sample = ...))
>   bitrate = ...
> else
>   bitrate = ctx->bit_rate;
>
hmm, I tried not to change the behaviour of the code I extracted as I  
only wanted to refactor and what you suggest would change the  
behaviour for CODEC_TYPE_UNKNOWN ,  CODEC_TYPE_ATTACHMENT and  
CODEC_TYPE_NB as it would no longer return 0 but return ctx->bit_rate.  
If that makes sense, I cannot judge. I'll change it accordingly, if  
you say it does.

>>
>
> Please split the patch, one which *defines* the new function, and
> which also updates the minor version of libavcodec, and one which uses
> the new function to factorize the code in utils.c (this can be sent
> once the first one is applied, use quilt / git / stgit / guilt if you
> want to work with stack of patches in your working copy).
>

OK, I'll do that, as soon as someone answers the one remaining  
question above.

Thanks for the review.

Cheers,

Robert