[FFmpeg-devel] [PATCH] ffmpeg: fix implementation of updated input start time

[Anton Khirnov]

Quoting Gyan Doshi (2022-09-27 06:40:56)
> The current adjustment of input start times just adjusts the tsoffset.
> And it does so, by resetting the tsoffset to nullify the new start time.
> This leads to breakage of -copyts, ignoring of user_ts_offset, breaking
                                                 ^^^^^^^^^^^^^^
                                                 no such variable
> of -isync as well as breaking wrap correction.

Given all these options that are supposed to interact with each other in
highly nonobvious ways, it would be great to have tests for the use
cases you're fixing here.

> Fixed by taking cognizance of these parameters, and by correcting start
> times just before sync offsets are applied.
> ---
>  fftools/ffmpeg.c       | 24 +-------------------
>  fftools/ffmpeg.h       |  5 ++++-
>  fftools/ffmpeg_demux.c |  4 ++--
>  fftools/ffmpeg_opt.c   | 50 +++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 52 insertions(+), 31 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 0e1477299d..fabb0fb952 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1898,7 +1898,7 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>          start_time = 0;
>          if (copy_ts) {
>              start_time += f->start_time != AV_NOPTS_VALUE ? f->start_time : 0;
> -            start_time += start_at_zero ? 0 : f->ctx->start_time;
> +            start_time += start_at_zero ? 0 : f->enabled_start_time;
>          }
>          if (ist->pts >= f->recording_time + start_time) {
>              close_output_stream(ost);
> @@ -3323,28 +3323,6 @@ static int transcode_init(void)
>                  input_streams[j + ifile->ist_index]->start = av_gettime_relative();
>      }
>  
> -    // Correct starttime based on the enabled streams
> -    for (i = 0; i < nb_input_files; i++) {
> -        InputFile       *ifile = input_files[i];
> -        AVFormatContext    *is = ifile->ctx;
> -        int64_t new_start_time = INT64_MAX;
> -
> -        if (is->start_time == AV_NOPTS_VALUE ||
> -            !(is->iformat->flags & AVFMT_TS_DISCONT))
> -            continue;
> -
> -        for (int j = 0; j < is->nb_streams; j++) {
> -            AVStream *st = is->streams[j];
> -            if(st->discard == AVDISCARD_ALL || st->start_time == AV_NOPTS_VALUE)
> -                continue;
> -            new_start_time = FFMIN(new_start_time, av_rescale_q(st->start_time, st->time_base, AV_TIME_BASE_Q));
> -        }
> -        if (new_start_time > is->start_time) {
> -            av_log(is, AV_LOG_VERBOSE, "Correcting start time by %"PRId64"\n", new_start_time - is->start_time);
> -            ifile->ts_offset = -new_start_time;
> -        }
> -    }
> -

The change would be more readable if you first moved this block into a
separate function in a separate patch.

Also, apply_sync_offsets() and correct_input_start_times() both modify
ts_offset in complicated ways. IMO it makes more sense to have both
loops in one function.

>      /* init input streams */
>      for (i = 0; i < nb_input_streams; i++)
>          if ((ret = init_input_stream(i, error, sizeof(error))) < 0)
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index ede0b2bd96..b93c2427b6 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -439,7 +439,10 @@ typedef struct InputFile {
>      AVRational time_base; /* time base of the duration */
>      int64_t input_ts_offset;
>      int input_sync_ref;
> -
> +    /**
> +     * Effective format start time based on enabled streams.
> +     */
> +    int64_t enabled_start_time;

Not a big fan of the name. Maybe start_time_effective would be better.
Or maybe even rename InputFile.start_time into start_time_user and reuse
the start_time name for the new variable.

-- 
Anton Khirnov