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

[Stefano Sabatini]

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.

> + *
> + * @ctx AVCodecContext of the stream

this is:
@param ctx ...

> + * @return Bit rate in bits per second or 0 if it could not be  
> determined

*b*it rate, lowcased is preferred here.

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

> +    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;

>  void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int 
> encode)
>  {
>      const char *codec_name;
> @@ -813,7 +841,6 @@
>              snprintf(buf + strlen(buf), buf_size - strlen(buf),
>                       ", q=%d-%d", enc->qmin, enc->qmax);
>          }
> -        bitrate = enc->bit_rate;
>          break;
>      case CODEC_TYPE_AUDIO:
>          snprintf(buf, buf_size,
> @@ -830,57 +857,15 @@
>                       ", %s", avcodec_get_sample_fmt_name(enc- 
> >sample_fmt));
>          }
>
> -        /* for PCM codecs, compute bitrate directly */
> -        switch(enc->codec_id) {
> -        case CODEC_ID_PCM_F64BE:
> -        case CODEC_ID_PCM_F64LE:
> -            bitrate = enc->sample_rate * enc->channels * 64;
> -            break;
> -        case CODEC_ID_PCM_S32LE:
> -        case CODEC_ID_PCM_S32BE:
> -        case CODEC_ID_PCM_U32LE:
> -        case CODEC_ID_PCM_U32BE:
> -        case CODEC_ID_PCM_F32BE:
> -        case CODEC_ID_PCM_F32LE:
> -            bitrate = enc->sample_rate * enc->channels * 32;
> -            break;
> -        case CODEC_ID_PCM_S24LE:
> -        case CODEC_ID_PCM_S24BE:
> -        case CODEC_ID_PCM_U24LE:
> -        case CODEC_ID_PCM_U24BE:
> -        case CODEC_ID_PCM_S24DAUD:
> -            bitrate = enc->sample_rate * enc->channels * 24;
> -            break;
> -        case CODEC_ID_PCM_S16LE:
> -        case CODEC_ID_PCM_S16BE:
> -        case CODEC_ID_PCM_S16LE_PLANAR:
> -        case CODEC_ID_PCM_U16LE:
> -        case CODEC_ID_PCM_U16BE:
> -            bitrate = enc->sample_rate * enc->channels * 16;
> -            break;
> -        case CODEC_ID_PCM_S8:
> -        case CODEC_ID_PCM_U8:
> -        case CODEC_ID_PCM_ALAW:
> -        case CODEC_ID_PCM_MULAW:
> -        case CODEC_ID_PCM_ZORK:
> -            bitrate = enc->sample_rate * enc->channels * 8;
> -            break;
> -        default:
> -            bitrate = enc->bit_rate;
> -            break;
> -        }
>          break;
>      case CODEC_TYPE_DATA:
>          snprintf(buf, buf_size, "Data: %s", codec_name);
> -        bitrate = enc->bit_rate;
>          break;
>      case CODEC_TYPE_SUBTITLE:
>          snprintf(buf, buf_size, "Subtitle: %s", codec_name);
> -        bitrate = enc->bit_rate;
>          break;
>      case CODEC_TYPE_ATTACHMENT:
>          snprintf(buf, buf_size, "Attachment: %s", codec_name);
> -        bitrate = enc->bit_rate;
>          break;
>      default:
>          snprintf(buf, buf_size, "Invalid Codec type %d", enc- 
> >codec_type);
> @@ -894,6 +879,7 @@
>              snprintf(buf + strlen(buf), buf_size - strlen(buf),
>                       ", pass 2");
>      }
> +    bitrate = av_get_bitrate(enc);
>      if (bitrate != 0) {
>          snprintf(buf + strlen(buf), buf_size - strlen(buf),
>                   ", %d kb/s", bitrate / 1000);

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

Regards and thanks for the patch.
-- 
FFmpeg = Fierce and Foolish Mysterious Portable Extravagant Ghost