[12/12] libopencore-amr, libvo-amrwbenc: Dynamically print the bitrate error message
diff mbox

Message ID 1302645498-12642-12-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö April 12, 2011, 9:58 p.m. 
---
 libavcodec/libopencore-amr.c |   18 ++++++++----------
 libavcodec/libvo-amrwbenc.c  |   17 ++++++-----------
 2 files changed, 14 insertions(+), 21 deletions(-)

Comments

Ronald Bultje April 12, 2011, 11:45 p.m. | #1 
Hi,

On Tue, Apr 12, 2011 at 5:58 PM, Martin Storsjö <martin@martin.st> wrote:
> ---
>  libavcodec/libopencore-amr.c |   18 ++++++++----------
>  libavcodec/libvo-amrwbenc.c  |   17 ++++++-----------
>  2 files changed, 14 insertions(+), 21 deletions(-)
[..]
> -static const char nb_bitrate_unsupported[] =
> -    "bitrate not supported: use one of 4.75k, 5.15k, 5.9k, 6.7k, 7.4k, 7.95k, 10.2k or 12.2k\n";
[..]
> +    av_log(log_ctx, AV_LOG_ERROR, "bitrate not supported: use one of ");
> +    for (i = 0; i < 8; i++)
> +        av_log(log_ctx, AV_LOG_ERROR, "%d%s", rates[i].rate,
> +                                              i < 7 ? ", " : "\n");

This isn't a good idea. 2 reasons:
- 1, what if threading is enabled? Threads could basically log
interleaved, and that would lead to garbage on the terminal
- 2, you're printing in decimals instead of in k units now.

What I'd prefer is to keep the nice output that you have now, maybe
put it in a temporary buffer before printing to the terminal (I know,
av_hex_dump_log() has that issue also and I have a patch for that
somewhere). Then lastly, if the bitrate is not right, simply select
the best one. Don't error out. Just print a warning. And then
continue.

Ronald
Martin Storsjö April 13, 2011, 8:10 a.m. | #2 
On Tue, 12 Apr 2011, Ronald S. Bultje wrote:

> On Tue, Apr 12, 2011 at 5:58 PM, Martin Storsjö <martin@martin.st> wrote:
> > ---
> >  libavcodec/libopencore-amr.c |   18 ++++++++----------
> >  libavcodec/libvo-amrwbenc.c  |   17 ++++++-----------
> >  2 files changed, 14 insertions(+), 21 deletions(-)
> [..]
> > -static const char nb_bitrate_unsupported[] =
> > -    "bitrate not supported: use one of 4.75k, 5.15k, 5.9k, 6.7k, 7.4k, 7.95k, 10.2k or 12.2k\n";
> [..]
> > +    av_log(log_ctx, AV_LOG_ERROR, "bitrate not supported: use one of ");
> > +    for (i = 0; i < 8; i++)
> > +        av_log(log_ctx, AV_LOG_ERROR, "%d%s", rates[i].rate,
> > +                                              i < 7 ? ", " : "\n");
> 
> This isn't a good idea. 2 reasons:
> - 1, what if threading is enabled? Threads could basically log
> interleaved, and that would lead to garbage on the terminal

I'm not sure if there actually is any guarantee that each log line will be 
atomic even if writing with a single av_log(), but there's at least less 
risk for messups. Changed

> - 2, you're printing in decimals instead of in k units now.

Changed.

> What I'd prefer is to keep the nice output that you have now, maybe
> put it in a temporary buffer before printing to the terminal (I know,
> av_hex_dump_log() has that issue also and I have a patch for that
> somewhere). Then lastly, if the bitrate is not right, simply select
> the best one. Don't error out. Just print a warning. And then
> continue.

Changed not to error out but just print a warning, and not to recheck and 
re-warn for each frame unless the user actually changed the bitrate 
inbetween, the rest of the patchset coming up.

// Martin

Patch
diff mbox 

diff --git a/libavcodec/libopencore-amr.c b/libavcodec/libopencore-amr.c
index e3708f1..9575916 100644
--- a/libavcodec/libopencore-amr.c
+++ b/libavcodec/libopencore-amr.c
@@ -40,9 +40,6 @@  static void amr_decode_fix_avctx(AVCodecContext *avctx)
 #include <opencore-amrnb/interf_dec.h>
 #include <opencore-amrnb/interf_enc.h>
 
-static const char nb_bitrate_unsupported[] =
-    "bitrate not supported: use one of 4.75k, 5.15k, 5.9k, 6.7k, 7.4k, 7.95k, 10.2k or 12.2k\n";
-
 /* Common code for fixed and float version*/
 typedef struct AMR_bitrates {
     int       rate;
@@ -50,7 +47,7 @@  typedef struct AMR_bitrates {
 } AMR_bitrates;
 
 /* Match desired bitrate */
-static int get_bitrate_mode(int bitrate)
+static int get_bitrate_mode(int bitrate, void *log_ctx)
 {
     /* make the correspondance between bitrate and mode */
     static const AMR_bitrates rates[] = {
@@ -63,6 +60,11 @@  static int get_bitrate_mode(int bitrate)
         if (rates[i].rate == bitrate)
             return rates[i].mode;
     /* no bitrate matching, return an error */
+    av_log(log_ctx, AV_LOG_ERROR, "bitrate not supported: use one of ");
+    for (i = 0; i < 8; i++)
+        av_log(log_ctx, AV_LOG_ERROR, "%d%s", rates[i].rate,
+                                              i < 7 ? ", " : "\n");
+
     return -1;
 }
 
@@ -161,10 +163,8 @@  static av_cold int amr_nb_encode_init(AVCodecContext *avctx)
         return -1;
     }
 
-    if ((s->enc_bitrate = get_bitrate_mode(avctx->bit_rate)) < 0) {
-        av_log(avctx, AV_LOG_ERROR, nb_bitrate_unsupported);
+    if ((s->enc_bitrate = get_bitrate_mode(avctx->bit_rate, avctx)) < 0)
         return AVERROR(ENOSYS);
-    }
 
     return 0;
 }
@@ -185,10 +185,8 @@  static int amr_nb_encode_frame(AVCodecContext *avctx,
     AMRContext *s = avctx->priv_data;
     int written;
 
-    if ((s->enc_bitrate = get_bitrate_mode(avctx->bit_rate)) < 0) {
-        av_log(avctx, AV_LOG_ERROR, nb_bitrate_unsupported);
+    if ((s->enc_bitrate = get_bitrate_mode(avctx->bit_rate, avctx)) < 0)
         return AVERROR(ENOSYS);
-    }
 
     written = Encoder_Interface_Encode(s->enc_state, s->enc_bitrate, data,
                                        frame, 0);
diff --git a/libavcodec/libvo-amrwbenc.c b/libavcodec/libvo-amrwbenc.c
index e55fdc6..1a2d8ef 100644
--- a/libavcodec/libvo-amrwbenc.c
+++ b/libavcodec/libvo-amrwbenc.c
@@ -23,17 +23,13 @@ 
 
 #include "avcodec.h"
 
-static const char wb_bitrate_unsupported[] =
-    "bitrate not supported: use one of 6.6k, 8.85k, 12.65k, 14.25k, 15.85k, "
-    "18.25k, 19.85k, 23.05k, or 23.85k\n";
-
 typedef struct AMRWBContext {
     void  *state;
     int    mode;
     int    allow_dtx;
 } AMRWBContext;
 
-static int get_wb_bitrate_mode(int bitrate)
+static int get_wb_bitrate_mode(int bitrate, void *log_ctx)
 {
     /* make the correspondance between bitrate and mode */
     static const int rates[] = {  6600,  8850, 12650, 14250, 15850, 18250,
@@ -44,6 +40,9 @@  static int get_wb_bitrate_mode(int bitrate)
         if (rates[i] == bitrate)
             return i;
     /* no bitrate matching, return an error */
+    av_log(log_ctx, AV_LOG_ERROR, "bitrate not supported: use one of ");
+    for (i = 0; i < 9; i++)
+        av_log(log_ctx, AV_LOG_ERROR, "%d%s", rates[i], i < 8 ? ", " : "\n");
     return -1;
 }
 
@@ -61,10 +60,8 @@  static av_cold int amr_wb_encode_init(AVCodecContext *avctx)
         return AVERROR(ENOSYS);
     }
 
-    if ((s->mode = get_wb_bitrate_mode(avctx->bit_rate)) < 0) {
-        av_log(avctx, AV_LOG_ERROR, wb_bitrate_unsupported);
+    if ((s->mode = get_wb_bitrate_mode(avctx->bit_rate, avctx)) < 0)
         return AVERROR(ENOSYS);
-    }
 
     avctx->frame_size  = 320;
     avctx->coded_frame = avcodec_alloc_frame();
@@ -91,10 +88,8 @@  static int amr_wb_encode_frame(AVCodecContext *avctx,
     AMRWBContext *s = avctx->priv_data;
     int size;
 
-    if ((s->mode = get_wb_bitrate_mode(avctx->bit_rate)) < 0) {
-        av_log(avctx, AV_LOG_ERROR, wb_bitrate_unsupported);
+    if ((s->mode = get_wb_bitrate_mode(avctx->bit_rate, avctx)) < 0)
         return AVERROR(ENOSYS);
-    }
     size = E_IF_encode(s->state, s->mode, data, frame, s->allow_dtx);
     return size;
 }