Demuxer reopening fixes

This is quite deep and important change. Thing is, old code had
"demuxer reuse" attempt. And unfortunately it neither made sense
(next to nothing performance improvement, complex code, limited
to MPEGTS streams), nor was working fine.

During Low Latency work I tried to eliminate demuxer reusing
because I wanted single demuxer logic to make input I/O plug-in
easier. It turned out that so modified code failed certain tests
such as Nvidia_AudioOnly.

It took me a great deal of debugging, but I found out that there
is bug in demuxer reuse procedure: namely only first muxer open
attempt _really_ checks what is in the segment provided, the rest
just assumes that same streams are there.

So basically, in the series of "audio/video"/"audio without video"
/"audio/video" segments all will be perceived as containing video!
This "patched over" *AudioOnly tests, and let them run, but only
because Transcoder "though" that it has video all the time.

And this simply wasn't true. When I removed "muxer reusing" code,
the Nvidia-dedicated "preserve video decoder" tactics was falling
over the fact that pixel format changed (because it changed from
AV_PIX_FMT_SOMETHING into AV_PIX_FMT_NONE) and tried to create
hardware video decoder for AV_PIX_FMT_NONE, and failed.

Fixing all that finally allowed me to get rid of recursive call to
open_input when HW decoder needs to be reset. Which is good.

During the work on this commit I also noticed that various pieces
of code here and there assume that video stream is always there.
Which is not true, and never was (for example, some tests were
using segments without video in the middle of transcode sequence,
and it worked only because Transcoder "failed to notice" there is
no video anymore). So the whole code was carefully tested and
examined against this dangerous assumption and checks were added.

These changes, in turn, allowed to remove the limitations that
allowed the Transcoder to start only with segments containing
video. Transcoder will now happily process audio-only segments.

Author: MikeIndiaAlpha

ffmpeg/api_test.go

@@ -88,7 +88,7 @@ func TestAPI_SkippedSegment(t *testing.T) {
 }
 
 func TestTranscoderAPI_InvalidFile(t *testing.T) {
-	// Test the following file open results on input: fail, success, fail, success
+	// Test the following file open results on input: success, fail, success
 
 	tc := NewTranscoder()
 	defer tc.StopTranscoder()
@@ -100,18 +100,9 @@ func TestTranscoderAPI_InvalidFile(t *testing.T) {
 		Muxer:        ComponentOptions{Name: "null"},
 	}}
 
-	// fail # 1
-	in.Fname = "none"
-	_, err := tc.Transcode(in, out)
-	if err == nil || err.Error() != "TranscoderInvalidVideo" {
-		// Early codec check didn't find video in missing input file so we get `TranscoderInvalidVideo`
-		//  instead of `No such file or directory`
-		t.Error("Expected 'TranscoderInvalidVideo', got ", err)
-	}
-
 	// success # 1
 	in.Fname = "../transcoder/test.ts"
-	_, err = tc.Transcode(in, out)
+	_, err := tc.Transcode(in, out)
 	if err != nil {
 		t.Error(err)
 	}
@@ -1217,27 +1208,6 @@ func audioOnlySegment(t *testing.T, accel Acceleration) {
 		}
 	}
 	tc.StopTranscoder()
-
-	// Test encoding with audio-only segment in start of stream
-	tc = NewTranscoder()
-	defer tc.StopTranscoder()
-	for i := 2; i < 4; i++ {
-		in := &TranscodeOptionsIn{
-			Fname: fmt.Sprintf("%s/test%d.ts", dir, i),
-			Accel: accel,
-		}
-		out := []TranscodeOptions{{
-			Oname:   fmt.Sprintf("%s/out2_%d.ts", dir, i),
-			Profile: prof,
-			Accel:   accel,
-		}}
-		_, err := tc.Transcode(in, out)
-		if i == 2 && (err == nil || err.Error() != "TranscoderInvalidVideo") {
-			t.Errorf("Expected to fail for audio-only segment but did not, instead got err=%v", err)
-		} else if i != 2 && err != nil {
-			t.Error(err)
-		}
-	}
 }
 
 func TestTranscoder_AudioOnly(t *testing.T) {

ffmpeg/decoder.c

@@ -128,29 +128,36 @@ static enum AVPixelFormat get_hw_pixfmt(AVCodecContext *vc, const enum AVPixelFo
 }
 
 
-int open_audio_decoder(struct input_ctx *ctx, AVCodec *codec)
+static int open_audio_decoder(struct input_ctx *ctx, AVCodec *codec)
 {
   int ret = 0;
   AVFormatContext *ic = ctx->ic;
 
   // open audio decoder
-    AVCodecContext * ac = avcodec_alloc_context3(codec);
-    if (!ac) LPMS_ERR(open_audio_err, "Unable to alloc audio codec");
-    if (ctx->ac) LPMS_WARN("An audio context was already open!");
-    ctx->ac = ac;
-    ret = avcodec_parameters_to_context(ac, ic->streams[ctx->ai]->codecpar);
-    if (ret < 0) LPMS_ERR(open_audio_err, "Unable to assign audio params");
-    ret = avcodec_open2(ac, codec, NULL);
-    if (ret < 0) LPMS_ERR(open_audio_err, "Unable to open audio decoder");
-
+  AVCodecContext * ac = avcodec_alloc_context3(codec);
+  if (!ac) LPMS_ERR(open_audio_err, "Unable to alloc audio codec");
+  if (ctx->ac) LPMS_WARN("An audio context was already open!");
+  ctx->ac = ac;
+  ret = avcodec_parameters_to_context(ac, ic->streams[ctx->ai]->codecpar);
+  if (ret < 0) LPMS_ERR(open_audio_err, "Unable to assign audio params");
+  ret = avcodec_open2(ac, codec, NULL);
+  if (ret < 0) LPMS_ERR(open_audio_err, "Unable to open audio decoder");
+  ctx->last_frame_a = av_frame_alloc();
+  if (!ctx->last_frame_a) LPMS_ERR(open_audio_err, "Unable to alloc last_frame_a");
   return 0;
 
 open_audio_err:
   free_input(ctx, FORCE_CLOSE_HW_DECODER);
   return ret;
 }
 
-char* get_hw_decoder(int ff_codec_id, int hw_type)
+static void close_audio_decoder(struct input_ctx *ictx)
+{
+  if (ictx->ac) avcodec_free_context(&ictx->ac);
+  if (ictx->last_frame_a) av_frame_free(&ictx->last_frame_a);
+}
+
+static char* get_hw_decoder(int ff_codec_id, int hw_type)
 {
     switch (hw_type) {
         case AV_HWDEVICE_TYPE_CUDA:
@@ -184,47 +191,51 @@ char* get_hw_decoder(int ff_codec_id, int hw_type)
     }
 }
 
-int open_video_decoder(struct input_ctx *ctx, AVCodec *codec)
+static int open_video_decoder(struct input_ctx *ctx, AVCodec *codec)
 {
   int ret = 0;
   AVDictionary **opts = NULL;
   AVFormatContext *ic = ctx->ic;
   // open video decoder
-    if (ctx->hw_type > AV_HWDEVICE_TYPE_NONE) {
-      char* decoder_name = get_hw_decoder(codec->id, ctx->hw_type);
-      if (!*decoder_name) {
-        ret = lpms_ERR_INPUT_CODEC;
-        LPMS_ERR(open_decoder_err, "Input codec does not support hardware acceleration");
-      }
-      AVCodec *c = avcodec_find_decoder_by_name(decoder_name);
-      if (c) codec = c;
-      else LPMS_WARN("Nvidia decoder not found; defaulting to software");
-      if (AV_PIX_FMT_YUV420P != ic->streams[ctx->vi]->codecpar->format &&
-          AV_PIX_FMT_YUVJ420P != ic->streams[ctx->vi]->codecpar->format) {
-        // TODO check whether the color range is truncated if yuvj420p is used
-        ret = lpms_ERR_INPUT_PIXFMT;
-        LPMS_ERR(open_decoder_err, "Non 4:2:0 pixel format detected in input");
-      }
+  if (ctx->hw_type > AV_HWDEVICE_TYPE_NONE) {
+    char* decoder_name = get_hw_decoder(codec->id, ctx->hw_type);
+    if (!*decoder_name) {
+      ret = lpms_ERR_INPUT_CODEC;
+      LPMS_ERR(open_decoder_err, "Input codec does not support hardware acceleration");
     }
-    AVCodecContext *vc = avcodec_alloc_context3(codec);
-    if (!vc) LPMS_ERR(open_decoder_err, "Unable to alloc video codec");
-    ctx->vc = vc;
-    ret = avcodec_parameters_to_context(vc, ic->streams[ctx->vi]->codecpar);
-    if (ret < 0) LPMS_ERR(open_decoder_err, "Unable to assign video params");
-    vc->opaque = (void*)ctx;
-    // XXX Could this break if the original device falls out of scope in golang?
-    if (ctx->hw_type == AV_HWDEVICE_TYPE_CUDA) {
-      // First set the hw device then set the hw frame
-      ret = av_hwdevice_ctx_create(&ctx->hw_device_ctx, ctx->hw_type, ctx->device, NULL, 0);
-      if (ret < 0) LPMS_ERR(open_decoder_err, "Unable to open hardware context for decoding")
-      vc->hw_device_ctx = av_buffer_ref(ctx->hw_device_ctx);
-      vc->get_format = get_hw_pixfmt;
+    AVCodec *c = avcodec_find_decoder_by_name(decoder_name);
+    if (c) codec = c;
+    else LPMS_WARN("Nvidia decoder not found; defaulting to software");
+    // It is safe to use ctx->vi here, because open_video_decoder won't be
+    // called if vi < 0
+    if (AV_PIX_FMT_YUV420P != ic->streams[ctx->vi]->codecpar->format &&
+        AV_PIX_FMT_YUVJ420P != ic->streams[ctx->vi]->codecpar->format) {
+      // TODO check whether the color range is truncated if yuvj420p is used
+      ret = lpms_ERR_INPUT_PIXFMT;
+      LPMS_ERR(open_decoder_err, "Non 4:2:0 pixel format detected in input");
     }
-    vc->pkt_timebase = ic->streams[ctx->vi]->time_base;
-    av_opt_set(vc->priv_data, "xcoder-params", ctx->xcoderParams, 0);
-    ret = avcodec_open2(vc, codec, opts);
-    if (ret < 0) LPMS_ERR(open_decoder_err, "Unable to open video decoder");
+  }
 
+  AVCodecContext *vc = avcodec_alloc_context3(codec);
+  if (!vc) LPMS_ERR(open_decoder_err, "Unable to alloc video codec");
+  ctx->vc = vc;
+  ret = avcodec_parameters_to_context(vc, ic->streams[ctx->vi]->codecpar);
+  if (ret < 0) LPMS_ERR(open_decoder_err, "Unable to assign video params");
+  vc->opaque = (void*)ctx;
+  // XXX Could this break if the original device falls out of scope in golang?
+  if (ctx->hw_type == AV_HWDEVICE_TYPE_CUDA) {
+    // First set the hw device then set the hw frame
+    ret = av_hwdevice_ctx_create(&ctx->hw_device_ctx, ctx->hw_type, ctx->device, NULL, 0);
+    if (ret < 0) LPMS_ERR(open_decoder_err, "Unable to open hardware context for decoding")
+    vc->hw_device_ctx = av_buffer_ref(ctx->hw_device_ctx);
+    vc->get_format = get_hw_pixfmt;
+  }
+  vc->pkt_timebase = ic->streams[ctx->vi]->time_base;
+  av_opt_set(vc->priv_data, "xcoder-params", ctx->xcoderParams, 0);
+  ret = avcodec_open2(vc, codec, opts);
+  if (ret < 0) LPMS_ERR(open_decoder_err, "Unable to open video decoder");
+  ctx->last_frame_v = av_frame_alloc();
+  if (!ctx->last_frame_v) LPMS_ERR(open_decoder_err, "Unable to alloc last_frame_v");
   return 0;
 
 open_decoder_err:
@@ -233,6 +244,16 @@ int open_video_decoder(struct input_ctx *ctx, AVCodec *codec)
   return ret;
 }
 
+static void close_video_decoder(struct input_ctx *ictx)
+{
+  if (ictx->vc) {
+    if (ictx->vc->hw_device_ctx) av_buffer_unref(&ictx->vc->hw_device_ctx);
+    avcodec_free_context(&ictx->vc);
+  }
+  if (ictx->hw_device_ctx) av_buffer_unref(&ictx->hw_device_ctx);
+  if (ictx->last_frame_v) av_frame_free(&ictx->last_frame_v);
+}
+
 int open_input(input_params *params, struct input_ctx *ctx)
 {
   char *inp = params->fname;
@@ -247,52 +268,51 @@ int open_input(input_params *params, struct input_ctx *ctx)
   ctx->device = params->device;
 
   // open demuxer
-  if (!ctx->ic) {
-    ret = avformat_open_input(&ctx->ic, inp, NULL, NULL);
-    if (ret < 0) LPMS_ERR(open_input_err, "demuxer: Unable to open input");
-    ret = avformat_find_stream_info(ctx->ic, NULL);
-    if (ret < 0) LPMS_ERR(open_input_err, "Unable to find input info");
-  } else if (!ctx->ic->pb) {
-    // reopen input segment file IO context if needed
-    ret = avio_open(&ctx->ic->pb, inp, AVIO_FLAG_READ);
-    if (ret < 0) LPMS_ERR(open_input_err, "Unable to reopen file");
-  } else reopen_decoders = 0;
+  ret = avformat_open_input(&ctx->ic, inp, NULL, NULL);
+  if (ret < 0) LPMS_ERR(open_input_err, "demuxer: Unable to open input");
+  ret = avformat_find_stream_info(ctx->ic, NULL);
+  if (ret < 0) LPMS_ERR(open_input_err, "Unable to find input info");
 
   AVCodec *video_codec = NULL;
   AVCodec *audio_codec = NULL;
   ctx->vi = av_find_best_stream(ctx->ic, AVMEDIA_TYPE_VIDEO, -1, -1, &video_codec, 0);
   ctx->ai = av_find_best_stream(ctx->ic, AVMEDIA_TYPE_AUDIO, -1, -1, &audio_codec, 0);
 
-  if (AV_HWDEVICE_TYPE_CUDA == ctx->hw_type && ctx->vi >= 0) {
-    if (ctx->last_format == AV_PIX_FMT_NONE) ctx->last_format = ctx->ic->streams[ctx->vi]->codecpar->format;
-    else if (ctx->ic->streams[ctx->vi]->codecpar->format != ctx->last_format) {
+  // Now be careful here. It appears that in certain situation (such as .ts
+  // stream without video stream) ctx->vi will be set to 0, but the format will
+  // be set to AV_PIX_FMT_NONE and both width and height will be zero, etc
+  // This is normally fine, but when re-using video decoder we have to be
+  // extra careful, and handle both situations: one with negative vi, and one
+  // with positive vi but AV_PIX_FMT_NONE in stream format
+  enum AVPixelFormat format =
+    (ctx->vi >= 0) ? ctx->ic->streams[ctx->vi]->codecpar->format : AV_PIX_FMT_NONE;
+  if ((AV_HWDEVICE_TYPE_CUDA == ctx->hw_type) && (ctx->vi >= 0)
+      && (AV_PIX_FMT_NONE != format)) {
+    if (ctx->last_format == AV_PIX_FMT_NONE) ctx->last_format = format;
+    else if (format != ctx->last_format) {
       LPMS_WARN("Input pixel format has been changed in the middle.");
-      ctx->last_format = ctx->ic->streams[ctx->vi]->codecpar->format;
+      ctx->last_format = format;
       // if the decoder is not re-opened when the video pixel format is changed,
       // the decoder tries HW decoding with the video context initialized to a pixel format different from the input one.
       // to handle a change in the input pixel format,
-      // we close the demuxer and re-open the decoder by calling open_input().
-      free_input(ctx, FORCE_CLOSE_HW_DECODER);
-      ret = open_input(params, ctx);
-      if (ret < 0) LPMS_ERR(open_input_err, "Unable to reopen video demuxer for HW decoding");
-      reopen_decoders = 0;
+      // we close the decoder so it will get reopened later
+      close_video_decoder(ctx);
     }
   }
 
   if (reopen_decoders) {
-    if (!ctx->dv && (ctx->vi >= 0) &&
-        (!ctx->vc || (ctx->hw_type == AV_HWDEVICE_TYPE_NONE))) {
-      ret = open_video_decoder(ctx, video_codec);
-      if (ret < 0) LPMS_ERR(open_input_err, "Unable to open video decoder")
-      ctx->last_frame_v = av_frame_alloc();
-      if (!ctx->last_frame_v) LPMS_ERR(open_input_err, "Unable to alloc last_frame_v");
+    if (!ctx->dv && (ctx->vi >= 0) && (AV_PIX_FMT_NONE != format)) {
+      // yes, we have video stream to decode, but check if we should reopen
+      // decoder
+      if (!ctx->vc || (ctx->hw_type == AV_HWDEVICE_TYPE_NONE)) {
+        ret = open_video_decoder(ctx, video_codec);
+        if (ret < 0) LPMS_ERR(open_input_err, "Unable to open video decoder")
+      }
     } else LPMS_WARN("No video stream found in input");
 
     if (!ctx->da && (ctx->ai >= 0)) {
       ret = open_audio_decoder(ctx, audio_codec);
       if (ret < 0) LPMS_ERR(open_input_err, "Unable to open audio decoder")
-      ctx->last_frame_a = av_frame_alloc();
-      if (!ctx->last_frame_a) LPMS_ERR(open_input_err, "Unable to alloc last_frame_a");
     } else LPMS_WARN("No audio stream found in input");
   }
 
@@ -306,44 +326,19 @@ int open_input(input_params *params, struct input_ctx *ctx)
 
 void free_input(struct input_ctx *ictx, enum FreeInputPolicy policy)
 {
-  if (FORCE_CLOSE_HW_DECODER == policy) {
-    // This means we are closing everything, so we also want to
-    // remove demuxer
-    if (ictx->ic) avformat_close_input(&ictx->ic);
-  } else {
-    // Otherwise we may want to retain demuxer in certain cases. Note that
-    // this is a lot of effort for very little gain, because demuxer is very
-    // cheap to create and destroy (being software component)
-    if (ictx->ic) {
-      // Only mpegts reuse the demuxer for subsequent segments.
-      // Close the demuxer for everything else.
-      // TODO might be reusable with fmp4 ; check!
-      if (!is_mpegts(ictx->ic)) avformat_close_input(&ictx->ic);
-      else if (ictx->ic->pb) {
-        // Reset leftovers from demuxer internals to prepare for next segment
-        avio_flush(ictx->ic->pb);
-        avformat_flush(ictx->ic);
-        avio_closep(&ictx->ic->pb);
-      }
-    }
-  }
+  if (ictx->ic) avformat_close_input(&ictx->ic);
   ictx->flushed = 0;
   ictx->flushing = 0;
   ictx->pkt_diff = 0;
   ictx->sentinel_count = 0;
+  // this is allocated elsewhere on first video packet
   if (ictx->first_pkt) av_packet_free(&ictx->first_pkt);
-  if (ictx->ac) avcodec_free_context(&ictx->ac);
   // video decoder is always closed when it is a SW decoder
   // otherwise only when forced
-  int close_vc = ictx->vc &&
-    ((AV_HWDEVICE_TYPE_NONE == ictx->hw_type) || (FORCE_CLOSE_HW_DECODER == policy));
-  if (close_vc) {
-    if (ictx->vc->hw_device_ctx) av_buffer_unref(&ictx->vc->hw_device_ctx);
-    avcodec_free_context(&ictx->vc);
-    if (ictx->hw_device_ctx) av_buffer_unref(&ictx->hw_device_ctx);
-    if (ictx->last_frame_v) av_frame_free(&ictx->last_frame_v);
+  if ((AV_HWDEVICE_TYPE_NONE == ictx->hw_type) || (FORCE_CLOSE_HW_DECODER == policy)) {
+    close_video_decoder(ictx);
   }
-  if (ictx->last_frame_a) av_frame_free(&ictx->last_frame_a);
-
+  // audio decoder is always closed
+  close_audio_decoder(ictx);
 }
 

ffmpeg/decoder.h

@@ -10,6 +10,9 @@ struct input_ctx {
   AVFormatContext *ic; // demuxer required
   AVCodecContext  *vc; // video decoder optional
   AVCodecContext  *ac; // audo  decoder optional
+  // TODO: perhaps get rid of indices and introduce pointers same way as on
+  // the encoder side, easier to check and easier to dereference without
+  // pointer to demuxer
   int vi, ai; // video and audio stream indices
   int dv, da; // flags whether to drop video or audio