From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 71A294446D for ; Fri, 14 Oct 2022 04:23:09 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C893768BD6C; Fri, 14 Oct 2022 07:23:06 +0300 (EEST) Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [80.241.56.152]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8E61F68BC2B for ; Fri, 14 Oct 2022 07:22:59 +0300 (EEST) Received: from smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:b231:465::2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4MpYCN31b3z9sRt for ; Fri, 14 Oct 2022 06:22:56 +0200 (CEST) Message-ID: <0a7a9061-9fd4-e187-799d-6afaa18319ab@gyani.pro> Date: Fri, 14 Oct 2022 09:52:42 +0530 MIME-Version: 1.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220927044056.2154-1-ffmpeg@gyani.pro> <166559164999.12287.9666902488178933378@lain.khirnov.net> From: Gyan Doshi In-Reply-To: <166559164999.12287.9666902488178933378@lain.khirnov.net> X-Rspamd-Queue-Id: 4MpYCN31b3z9sRt Subject: Re: [FFmpeg-devel] [PATCH] ffmpeg: fix implementation of updated input start time X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".