avconv: fix guessed channel layout being lost during stream copy

Message ID 20160918170752.3288-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Sept. 18, 2016, 5:07 p.m.
The guessed layout was being stored in the decoder context, which in the case of
stream copy is unused.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Didn't run FATE after this patch, but aside from guess_input_channel_layout()
nothing seemed to be using ist->dec_ctx inside the switch statement, so moving
the avcodec_parameters_to_context() call down shouldn't break anything.

Unless i'm missing something, this may be the first case of an input codecpar
being modified outside of the demuxer that filled it.
I assume this is acceptable, otherwise the solution would probably be more
complex or less clean.

 avconv.c     |  2 +-
 avconv_opt.c | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Diego Biurrun Sept. 19, 2016, 9:27 a.m. | #1
On Sun, Sep 18, 2016 at 02:07:52PM -0300, James Almer wrote:
> The guessed layout was being stored in the decoder context, which in the case of
> stream copy is unused.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Didn't run FATE after this patch, but aside from guess_input_channel_layout()
> nothing seemed to be using ist->dec_ctx inside the switch statement, so moving
> the avcodec_parameters_to_context() call down shouldn't break anything.

It does pass FATE. Please run FATE yourself next time.

Diego
Anton Khirnov Sept. 20, 2016, 7:21 p.m. | #2
Quoting James Almer (2016-09-18 19:07:52)
> The guessed layout was being stored in the decoder context, which in the case of
> stream copy is unused.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Didn't run FATE after this patch, but aside from guess_input_channel_layout()
> nothing seemed to be using ist->dec_ctx inside the switch statement, so moving
> the avcodec_parameters_to_context() call down shouldn't break anything.
> 
> Unless i'm missing something, this may be the first case of an input codecpar
> being modified outside of the demuxer that filled it.
> I assume this is acceptable, otherwise the solution would probably be more
> complex or less clean.

I tend to disagree here -- the doxy in lavf says that when demuxing
codecpar is filled by lavf. I would interpret this as meaning that the
caller is not allowed to randomly modify it and the demuxer may rely on
the information in it staying the same.

This specific change probably won't break any actual demuxers, but as
avconv tends to be seen as the "reference API user", I'd prefer it to
use the API correctly. It shouldn't be much work to make a private copy
of the codecpar and modify that instead.

Patch

diff --git a/avconv.c b/avconv.c
index 59eb300..0390b47 100644
--- a/avconv.c
+++ b/avconv.c
@@ -1310,7 +1310,7 @@  static int decode(AVCodecContext *avctx, AVFrame *frame, int *got_frame, AVPacke
 
 int guess_input_channel_layout(InputStream *ist)
 {
-    AVCodecContext *dec = ist->dec_ctx;
+    AVCodecParameters *dec = ist->st->codecpar;
 
     if (!dec->channel_layout) {
         char layout_name[256];
diff --git a/avconv_opt.c b/avconv_opt.c
index 362a5b7..d9c2318 100644
--- a/avconv_opt.c
+++ b/avconv_opt.c
@@ -545,12 +545,6 @@  static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
             exit_program(1);
         }
 
-        ret = avcodec_parameters_to_context(ist->dec_ctx, par);
-        if (ret < 0) {
-            av_log(NULL, AV_LOG_ERROR, "Error initializing the decoder context.\n");
-            exit_program(1);
-        }
-
         switch (par->codec_type) {
         case AVMEDIA_TYPE_VIDEO:
             MATCH_PER_STREAM_OPT(frame_rates, str, framerate, ic, st);
@@ -621,6 +615,12 @@  static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
         default:
             abort();
         }
+
+        ret = avcodec_parameters_to_context(ist->dec_ctx, par);
+        if (ret < 0) {
+            av_log(NULL, AV_LOG_ERROR, "Error initializing the decoder context.\n");
+            exit_program(1);
+        }
     }
 }