[FFmpeg-devel,4/4] avformat/concatdec: always re-calculate start time and duration

This allows the underlying files to change their duration on subsequent
avformat context opens.

An example use case where this matters:

ffconcat version 1.0
file dummy.mxf
file dummy.mxf

ffmpeg -re -stream_loop -1 -i dummy.ffconcat -f sdl2 none

The user can seamlessly change the input by atomically replacing dummy.mxf.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/concatdec.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Marton Balint (2018-11-22):
> This allows the underlying files to change their duration on subsequent
> avformat context opens.
> 
> An example use case where this matters:
> 
> ffconcat version 1.0
> file dummy.mxf
> file dummy.mxf
> 
> ffmpeg -re -stream_loop -1 -i dummy.ffconcat -f sdl2 none
> 
> The user can seamlessly change the input by atomically replacing dummy.mxf.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/concatdec.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> index ebc50324cc..2ebd2120c3 100644
> --- a/libavformat/concatdec.c
> +++ b/libavformat/concatdec.c
> @@ -355,14 +355,12 @@ static int open_file(AVFormatContext *avf, unsigned fileno)
>          return ret;
>      }
>      cat->cur_file = file;
> -    if (file->start_time == AV_NOPTS_VALUE)
> -        file->start_time = !fileno ? 0 :
> -                           cat->files[fileno - 1].start_time +
> -                           cat->files[fileno - 1].duration;
> +    file->start_time = !fileno ? 0 :
> +                       cat->files[fileno - 1].start_time +
> +                       cat->files[fileno - 1].duration;
>      file->file_start_time = (cat->avf->start_time == AV_NOPTS_VALUE) ? 0 : cat->avf->start_time;
>      file->file_inpoint = (file->inpoint == AV_NOPTS_VALUE) ? file->file_start_time : file->inpoint;
> -    if (file->duration == AV_NOPTS_VALUE)
> -        file->duration = get_best_effort_duration(file, cat->avf);
> +    file->duration = get_best_effort_duration(file, cat->avf);
>  
>      if (cat->segment_time_metadata) {
>          av_dict_set_int(&file->metadata, "lavf.concatdec.start_time", file->start_time, 0);
> @@ -529,8 +527,7 @@ static int open_next_file(AVFormatContext *avf)
>      ConcatContext *cat = avf->priv_data;
>      unsigned fileno = cat->cur_file - cat->files;
>  
> -    if (cat->cur_file->duration == AV_NOPTS_VALUE)
> -        cat->cur_file->duration = get_best_effort_duration(cat->cur_file, cat->avf);
> +    cat->cur_file->duration = get_best_effort_duration(cat->cur_file, cat->avf);
>  
>      if (++fileno >= cat->nb_files) {
>          cat->eof = 1;

I do not think it works. If the duration of the second file changes, for
example, then the start time of all subsequent files changes too, but
this patch does not update it. Seeking will show strange results.

Regards,

On Sun, 9 Dec 2018, Nicolas George wrote:

> Marton Balint (2018-11-22):
>> This allows the underlying files to change their duration on subsequent
>> avformat context opens.
>>
>> An example use case where this matters:
>>
>> ffconcat version 1.0
>> file dummy.mxf
>> file dummy.mxf
>>
>> ffmpeg -re -stream_loop -1 -i dummy.ffconcat -f sdl2 none
>>
>> The user can seamlessly change the input by atomically replacing dummy.mxf.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/concatdec.c | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
>> index ebc50324cc..2ebd2120c3 100644
>> --- a/libavformat/concatdec.c
>> +++ b/libavformat/concatdec.c
>> @@ -355,14 +355,12 @@ static int open_file(AVFormatContext *avf, unsigned fileno)
>>          return ret;
>>      }
>>      cat->cur_file = file;
>> -    if (file->start_time == AV_NOPTS_VALUE)
>> -        file->start_time = !fileno ? 0 :
>> -                           cat->files[fileno - 1].start_time +
>> -                           cat->files[fileno - 1].duration;
>> +    file->start_time = !fileno ? 0 :
>> +                       cat->files[fileno - 1].start_time +
>> +                       cat->files[fileno - 1].duration;
>>      file->file_start_time = (cat->avf->start_time == AV_NOPTS_VALUE) ? 0 : cat->avf->start_time;
>>      file->file_inpoint = (file->inpoint == AV_NOPTS_VALUE) ? file->file_start_time : file->inpoint;
>> -    if (file->duration == AV_NOPTS_VALUE)
>> -        file->duration = get_best_effort_duration(file, cat->avf);
>> +    file->duration = get_best_effort_duration(file, cat->avf);
>>
>>      if (cat->segment_time_metadata) {
>>          av_dict_set_int(&file->metadata, "lavf.concatdec.start_time", file->start_time, 0);
>> @@ -529,8 +527,7 @@ static int open_next_file(AVFormatContext *avf)
>>      ConcatContext *cat = avf->priv_data;
>>      unsigned fileno = cat->cur_file - cat->files;
>>
>> -    if (cat->cur_file->duration == AV_NOPTS_VALUE)
>> -        cat->cur_file->duration = get_best_effort_duration(cat->cur_file, cat->avf);
>> +    cat->cur_file->duration = get_best_effort_duration(cat->cur_file, cat->avf);
>>
>>      if (++fileno >= cat->nb_files) {
>>          cat->eof = 1;
>
> I do not think it works. If the duration of the second file changes, for
> example, then the start time of all subsequent files changes too, but
> this patch does not update it. Seeking will show strange results.

Seeking will only work in the special case I provided in the commit 
message, when we always seek back to the beginning and otherwise read the 
referenced files continously.

In theory, when we are in "seekable" state then if we detect a duration 
change we could indeed update all subsequent start times to provide proper 
timestamps for seeking, but I did not need this for my purpose, and I am 
not sure if it's worth the extra code needed.

Regards,
Marton

Marton Balint (2018-12-09):
> Seeking will only work in the special case I provided in the commit message,
> when we always seek back to the beginning and otherwise read the referenced
> files continously.
> 
> In theory, when we are in "seekable" state then if we detect a duration
> change we could indeed update all subsequent start times to provide proper
> timestamps for seeking, but I did not need this for my purpose, and I am not
> sure if it's worth the extra code needed.

I am really not comfortable with leaving the loaded gun of the seekable
flag if it does not work reliably. I think at the very least the code
should detect a file change, emit a warning and invalidate all
subsequent durations.

Note that if all you are interested in is seeking to the beginning of
the virtual file, it can always work. I would not object to a special
case in the seek code for that.

Regards,

On Thu, 13 Dec 2018, Nicolas George wrote:

> Marton Balint (2018-12-09):
>> Seeking will only work in the special case I provided in the commit message,
>> when we always seek back to the beginning and otherwise read the referenced
>> files continously.
>>
>> In theory, when we are in "seekable" state then if we detect a duration
>> change we could indeed update all subsequent start times to provide proper
>> timestamps for seeking, but I did not need this for my purpose, and I am not
>> sure if it's worth the extra code needed.
>
> I am really not comfortable with leaving the loaded gun of the seekable
> flag if it does not work reliably. I think at the very least the code
> should detect a file change, emit a warning and invalidate all
> subsequent durations.
>
> Note that if all you are interested in is seeking to the beginning of
> the virtual file, it can always work. I would not object to a special
> case in the seek code for that.

I can do this to replace the functionality of the first patch in the 
series. However, this patch (4/4) is still going to be needed, but the 
context will not be seekable anymore (except for seeking into start). Is 
this acceptable?

Thanks,
Marton