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

Submitted by Marton Balint on Dec. 30, 2018, 9:50 p.m.

Details

Message ID 20181230215001.20717-1-cus@passwd.hu
State Accepted
Commit 1b126ec4087ab5d87d413116bee666495b0d2d3e
Headers show

Commit Message

Marton Balint Dec. 30, 2018, 9:50 p.m.
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.

v2: Set ConcatFile duration in read_header for all segments with known
durations because from now on we always recalculate the start time in
open_file, and an instant seek could have caused unset ConcatFile durations.

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

Comments

Nicolas George Jan. 4, 2019, 1:41 p.m.
Marton Balint (12018-12-30):
> 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.
> 
> v2: Set ConcatFile duration in read_header for all segments with known
> durations because from now on we always recalculate the start time in
> open_file, and an instant seek could have caused unset ConcatFile durations.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/concatdec.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

I am sorry, but it still feels like a fragile solution, and I think it
can leave the context in an inconsistent state. Maybe it works now, but
it is a bad surprise left for somebody who wants to extend this demuxer
in another way later.

Also, I think the duration detected from the file should never override
the duration provided by the script.

What about this:

	if the user_duration is set to a special value then
	  each time the file is opened and closed
	    if the duration has changed
	      update the duration
	      update the start time of all subsequent files

?

The "user_duration is set to a special value" condition also addresses
another concern I have: this feature conflicts with the (net yet
implemented) option of keeping the files open, as a LRU-style
optimization for when much seeking is expected.

Regards,
Marton Balint Jan. 4, 2019, 9:37 p.m.
On Fri, 4 Jan 2019, Nicolas George wrote:

> Marton Balint (12018-12-30):
>> 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.
>>
>> v2: Set ConcatFile duration in read_header for all segments with known
>> durations because from now on we always recalculate the start time in
>> open_file, and an instant seek could have caused unset ConcatFile durations.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/concatdec.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> I am sorry, but it still feels like a fragile solution, and I think it
> can leave the context in an inconsistent state. Maybe it works now, but
> it is a bad surprise left for somebody who wants to extend this demuxer
> in another way later.
>
> Also, I think the duration detected from the file should never override
> the duration provided by the script.

Agreed, and this is how it should work even after the patch. user_duration 
which is the duration specified in the ffconcat file always have priority 
because get_best_effort_duration always considers that first. So it does 
not matter that we assign ConcatFile->duration again and again, because we 
will just reassing the same value.

Given that, I don't think an inconsitent state is possible because seeking 
is only allowed if the durations are known (if they are all set in 
the ffconcat file). On the other hand, if generic seeking is not allowed, 
then start_time will never be invalid because we always recalculate it for 
the next file when we open it.

Does this relax your concern?

Thanks,
Marton

>
> What about this:
>
> 	if the user_duration is set to a special value then
> 	  each time the file is opened and closed
> 	    if the duration has changed
> 	      update the duration
> 	      update the start time of all subsequent files
>
> ?
>
> The "user_duration is set to a special value" condition also addresses
> another concern I have: this feature conflicts with the (net yet
> implemented) option of keeping the files open, as a LRU-style
> optimization for when much seeking is expected.

Knowing the above
>
> Regards,
>
> --
>  Nicolas George
>
Marton Balint Jan. 19, 2019, 10:44 p.m.
On Fri, 4 Jan 2019, Marton Balint wrote:

>
>
> On Fri, 4 Jan 2019, Nicolas George wrote:
>
>> Marton Balint (12018-12-30):
>>> 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.
>>>
>>> v2: Set ConcatFile duration in read_header for all segments with known
>>> durations because from now on we always recalculate the start time in
>>> open_file, and an instant seek could have caused unset ConcatFile 
> durations.
>>>
>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>> ---
>>>  libavformat/concatdec.c | 14 ++++++--------
>>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> I am sorry, but it still feels like a fragile solution, and I think it
>> can leave the context in an inconsistent state. Maybe it works now, but
>> it is a bad surprise left for somebody who wants to extend this demuxer
>> in another way later.
>>
>> Also, I think the duration detected from the file should never override
>> the duration provided by the script.
>
> Agreed, and this is how it should work even after the patch. user_duration 
> which is the duration specified in the ffconcat file always have priority 
> because get_best_effort_duration always considers that first. So it does 
> not matter that we assign ConcatFile->duration again and again, because we 
> will just reassing the same value.
>
> Given that, I don't think an inconsitent state is possible because seeking 
> is only allowed if the durations are known (if they are all set in 
> the ffconcat file). On the other hand, if generic seeking is not allowed, 
> then start_time will never be invalid because we always recalculate it for 
> the next file when we open it.
>
> Does this relax your concern?
>

Ping.

Thanks,
Marton
Nicolas George Jan. 21, 2019, 2:47 p.m.
Marton Balint (12019-01-04):
> Agreed, and this is how it should work even after the patch. user_duration
> which is the duration specified in the ffconcat file always have priority
> because get_best_effort_duration always considers that first. So it does not
> matter that we assign ConcatFile->duration again and again, because we will
> just reassing the same value.
> 
> Given that, I don't think an inconsitent state is possible because seeking
> is only allowed if the durations are known (if they are all set in the
> ffconcat file). On the other hand, if generic seeking is not allowed, then
> start_time will never be invalid because we always recalculate it for the
> next file when we open it.
> 
> Does this relax your concern?

I think it does. Sorry for forgetting to reply to this. Please go ahead
when convenient.

Regards,
Marton Balint Jan. 22, 2019, 7:37 p.m.
On Mon, 21 Jan 2019, Nicolas George wrote:

> Marton Balint (12019-01-04):
>> Agreed, and this is how it should work even after the patch. user_duration
>> which is the duration specified in the ffconcat file always have priority
>> because get_best_effort_duration always considers that first. So it does not
>> matter that we assign ConcatFile->duration again and again, because we will
>> just reassing the same value.
>>
>> Given that, I don't think an inconsitent state is possible because seeking
>> is only allowed if the durations are known (if they are all set in the
>> ffconcat file). On the other hand, if generic seeking is not allowed, then
>> start_time will never be invalid because we always recalculate it for the
>> next file when we open it.
>>
>> Does this relax your concern?
>
> I think it does. Sorry for forgetting to reply to this. Please go ahead
> when convenient.

Thanks, pushed the remaining parts of the series.

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 890a220a89..d65da553e1 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);
@@ -504,6 +502,7 @@  static int concat_read_header(AVFormatContext *avf)
                 break;
             cat->files[i].user_duration = cat->files[i].outpoint - cat->files[i].inpoint;
         }
+        cat->files[i].duration = cat->files[i].user_duration;
         time += cat->files[i].user_duration;
     }
     if (i == cat->nb_files) {
@@ -529,8 +528,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;