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

[Gyan Doshi]

On 2022-10-12 09:50 pm, Anton Khirnov wrote:
> 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

Will correct.
>> 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.

These options don't really interact with each other. They all derive 
from or reference the start time.
Since ctx->start_time can't be changed outside lavf, we need to point to 
an fftools field. Basically, that's it.

>> 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.
Will do.

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

I'd prefer a parent container with these two as child functions. I like 
the one purpose per function - cleaner for any future changes.

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

I like start_time_effective . Will do.

Regards,
Gyan