[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.
>
fixed

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

>> + * @return Bit rate in bits per second or 0 if it could not be
>> determined
>
> *b*it rate, lowcased is preferred here.
>
Kept it uppercase due to Diego's feedback

>> + */
>> +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.
>
fixed

>> +    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;
>
I kept it as it is for the following reasons:
- I still don't know what to return for CODEC_TYPE_UNKNOWN and this is  
closest to what the code did that I factored it out of
- I found it a bit awkward to call av_get_bits_per_sample for every  
codec type although it seems to be used only for audio

is that OK anyway?

>
> 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).
>

this is this first patch. I'll send the other one, once this one has  
been applied.

> Regards and thanks for the patch.

thanks again for reviewing

Robert

P.S.: I hope the attached patch (yes, it is there) makes it through  
the mailing list software this time.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: av_get_bit_rate.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091112/7d8003fc/attachment.txt>
-------------- next part --------------