| Message ID | 20181230215001.20717-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | 1b126ec4087ab5d87d413116bee666495b0d2d3e |
| Headers | show |
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,
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 >
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
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,
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
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;
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(-)