| Message ID | 1469537773-29853-1-git-send-email-anssi.hannula@iki.fi |
|---|---|
| State | Accepted |
| Commit | 60873bf992eab1d3bad8dd0fd11336363d44854d |
| Headers | show |
On Tue, Jul 26, 2016 at 03:56:13PM +0300, Anssi Hannula wrote: > Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with > AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed > avformat_find_stream_info() to put the extradata it got from > st->parser->parser->split() to st->internal->avctx instead of st->codec > (from where it will be later copied to st->codecpar). > > However, in the same function, the "is stream ready?" check was changed > to check for extradata in st->codecpar instead of st->codec. > > Extradata retrieved from split() is therefore not considered anymore, > and avformat_find_stream_info() will therefore needlessly continue > probing in some cases. > > Fix that by checking for the extradata at st->internal->avctx where it > is actually put. > > --- > > Michael Niedermayer wrote: > > seems to break fate here: > [...] > > Oops, seems I messed up running fate and missed the "warning: only a > subset of the fate tests will be run because SAMPLES is not specified" > warning it gave... Thanks for catching that. > > Seems this reverse fix is actually needed, as extradata is actually > copied in the other direction (from st->internal->avctx to > st->codecpar). it seems this causes some changes, quite possibly thats simply due to less packets being read libavformat/tests/seek Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv -duration 400 with http://samples.ffmpeg.org/Matroska/matrix/Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv for tickets//2254/ttvHD_vlc_sample.ts the tbr changes from 1001/30000 to 1/30 are these in intended / expected ? I can confirm that this reduces the amount of data read in many files [...]
26.07.2016, 17:59, Michael Niedermayer kirjoitti: > On Tue, Jul 26, 2016 at 03:56:13PM +0300, Anssi Hannula wrote: >> Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with >> AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed >> avformat_find_stream_info() to put the extradata it got from >> st->parser->parser->split() to st->internal->avctx instead of st->codec >> (from where it will be later copied to st->codecpar). >> >> However, in the same function, the "is stream ready?" check was changed >> to check for extradata in st->codecpar instead of st->codec. >> >> Extradata retrieved from split() is therefore not considered anymore, >> and avformat_find_stream_info() will therefore needlessly continue >> probing in some cases. >> >> Fix that by checking for the extradata at st->internal->avctx where it >> is actually put. >> >> --- >> >> Michael Niedermayer wrote: >>> seems to break fate here: >> [...] >> >> Oops, seems I messed up running fate and missed the "warning: only a >> subset of the fate tests will be run because SAMPLES is not specified" >> warning it gave... Thanks for catching that. >> >> Seems this reverse fix is actually needed, as extradata is actually >> copied in the other direction (from st->internal->avctx to >> st->codecpar). > > it seems this causes some changes, quite possibly thats simply due to > less packets being read > > libavformat/tests/seek Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv -duration 400 > with http://samples.ffmpeg.org/Matroska/matrix/Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv @@ -9,7 +9,7 @@ ret: 0 st:13 flags:1 dts: 86.750000 pts: 86.750000 pos: -1 size: 50436 ret:-1 st: 1 flags:0 ts: 250.577000 ret: 0 st: 1 flags:1 ts: 13.471000 -ret: 0 st:13 flags:1 dts: 5.909000 pts: 5.909000 pos: -1 size: 50436 +ret: 0 st:13 flags:1 dts: 1.776000 pts: 1.776000 pos: -1 size: 50436 ret:-1 st: 2 flags:0 ts: 176.365000 ret: 0 st: 2 flags:1 ts: 339.259000 ret: 0 st:13 flags:1 dts: 145.080000 pts: 145.080000 pos: -1 size: 50436 Seems to indeed be due to less packets being read in avformat_find_stream_info(). Matroska demuxer seems to add keyframes to the index as it encounters them, and in this case more keyframes are encountered in avformat_stream_info() phase, which seems to result in more accurate seeking in the early seconds of the file. > for tickets//2254/ttvHD_vlc_sample.ts the tbr changes from > 1001/30000 to 1/30 Looks like the timestamps are jittery in that sample: 36072.231911, dts = prev + 3002 36072.265289, dts = prev + 3004 36072.298656, dts = prev + 3003 36072.331433, dts = prev + 2950 36072.364800, dts = prev + 3003 36072.398167, dts = prev + 3003 36072.431533, dts = prev + 3003 36072.464900, dts = prev + 3003 36072.498267, dts = prev + 3003 36072.531633, dts = prev + 3003 36072.565000, dts = prev + 3003 36072.598367, dts = prev + 3003 36072.630667, dts = prev + 2907 36072.664033, dts = prev + 3003 36072.697400, dts = prev + 3003 36072.730767, dts = prev + 3003 36072.764133, dts = prev + 3003 36072.797500, dts = prev + 3003 36072.830867, dts = prev + 3003 36072.864233, dts = prev + 3003 36072.897600, dts = prev + 3003 36072.939278, dts = prev + 3751 36072.972644, dts = prev + 3003 36073.006011, dts = prev + 3003 36073.034478, dts = prev + 2562 36073.067844, dts = prev + 3003 And this throws the ff_rfps_calculate() algorithm off unless it is called with fpsprobesize (fps_analyze_framecount) of 160 or more (default is 20). Not sure if something could/should be done to keep tbr of that file to be detected as 1001/3000. For both of these cases I verified that FFmpeg 3.0 had the same behavior as with this patch, i.e. their behavior was unintendedly changed by the original commit 6f69f7a8bf6a0d01. > are these in intended / expected ? > > I can confirm that this reduces the amount of data read in many files
On Tue, Jul 26, 2016 at 08:39:03PM +0300, Anssi Hannula wrote: > 26.07.2016, 17:59, Michael Niedermayer kirjoitti: > > On Tue, Jul 26, 2016 at 03:56:13PM +0300, Anssi Hannula wrote: > >> Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with > >> AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed > >> avformat_find_stream_info() to put the extradata it got from > >> st->parser->parser->split() to st->internal->avctx instead of st->codec > >> (from where it will be later copied to st->codecpar). > >> > >> However, in the same function, the "is stream ready?" check was changed > >> to check for extradata in st->codecpar instead of st->codec. > >> > >> Extradata retrieved from split() is therefore not considered anymore, > >> and avformat_find_stream_info() will therefore needlessly continue > >> probing in some cases. > >> > >> Fix that by checking for the extradata at st->internal->avctx where it > >> is actually put. > >> > >> --- > >> > >> Michael Niedermayer wrote: > >>> seems to break fate here: > >> [...] > >> > >> Oops, seems I messed up running fate and missed the "warning: only a > >> subset of the fate tests will be run because SAMPLES is not specified" > >> warning it gave... Thanks for catching that. > >> > >> Seems this reverse fix is actually needed, as extradata is actually > >> copied in the other direction (from st->internal->avctx to > >> st->codecpar). > > > > it seems this causes some changes, quite possibly thats simply due to > > less packets being read > > > > libavformat/tests/seek Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv -duration 400 > > with http://samples.ffmpeg.org/Matroska/matrix/Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv > > @@ -9,7 +9,7 @@ > ret: 0 st:13 flags:1 dts: 86.750000 pts: 86.750000 pos: -1 > size: 50436 > ret:-1 st: 1 flags:0 ts: 250.577000 > ret: 0 st: 1 flags:1 ts: 13.471000 > -ret: 0 st:13 flags:1 dts: 5.909000 pts: 5.909000 pos: -1 > size: 50436 > +ret: 0 st:13 flags:1 dts: 1.776000 pts: 1.776000 pos: -1 > size: 50436 > ret:-1 st: 2 flags:0 ts: 176.365000 > ret: 0 st: 2 flags:1 ts: 339.259000 > ret: 0 st:13 flags:1 dts: 145.080000 pts: 145.080000 pos: > -1 size: 50436 > > Seems to indeed be due to less packets being read in > avformat_find_stream_info(). > > Matroska demuxer seems to add keyframes to the index as it encounters > them, and in this case more keyframes are encountered in > avformat_stream_info() phase, which seems to result in more accurate > seeking in the early seconds of the file. > > > for tickets//2254/ttvHD_vlc_sample.ts the tbr changes from > > 1001/30000 to 1/30 > > Looks like the timestamps are jittery in that sample: > > 36072.231911, dts = prev + 3002 > 36072.265289, dts = prev + 3004 > 36072.298656, dts = prev + 3003 > 36072.331433, dts = prev + 2950 > 36072.364800, dts = prev + 3003 > 36072.398167, dts = prev + 3003 > 36072.431533, dts = prev + 3003 > 36072.464900, dts = prev + 3003 > 36072.498267, dts = prev + 3003 > 36072.531633, dts = prev + 3003 > 36072.565000, dts = prev + 3003 > 36072.598367, dts = prev + 3003 > 36072.630667, dts = prev + 2907 > 36072.664033, dts = prev + 3003 > 36072.697400, dts = prev + 3003 > 36072.730767, dts = prev + 3003 > 36072.764133, dts = prev + 3003 > 36072.797500, dts = prev + 3003 > 36072.830867, dts = prev + 3003 > 36072.864233, dts = prev + 3003 > 36072.897600, dts = prev + 3003 > 36072.939278, dts = prev + 3751 > 36072.972644, dts = prev + 3003 > 36073.006011, dts = prev + 3003 > 36073.034478, dts = prev + 2562 > 36073.067844, dts = prev + 3003 > > And this throws the ff_rfps_calculate() algorithm off unless it is > called with fpsprobesize (fps_analyze_framecount) of 160 or more > (default is 20). > > Not sure if something could/should be done to keep tbr of that file to > be detected as 1001/3000. > > > For both of these cases I verified that FFmpeg 3.0 had the same behavior > as with this patch, i.e. their behavior was unintendedly changed by the > original commit 6f69f7a8bf6a0d01. thanks alot for the deep analysis i have no objections to the change [...]
27.07.2016, 02:04, Michael Niedermayer kirjoitti: > On Tue, Jul 26, 2016 at 08:39:03PM +0300, Anssi Hannula wrote: >> 26.07.2016, 17:59, Michael Niedermayer kirjoitti: >>> On Tue, Jul 26, 2016 at 03:56:13PM +0300, Anssi Hannula wrote: >>>> Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with >>>> AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed >>>> avformat_find_stream_info() to put the extradata it got from >>>> st->parser->parser->split() to st->internal->avctx instead of st->codec >>>> (from where it will be later copied to st->codecpar). >>>> >>>> However, in the same function, the "is stream ready?" check was changed >>>> to check for extradata in st->codecpar instead of st->codec. >>>> >>>> Extradata retrieved from split() is therefore not considered anymore, >>>> and avformat_find_stream_info() will therefore needlessly continue >>>> probing in some cases. >>>> >>>> Fix that by checking for the extradata at st->internal->avctx where it >>>> is actually put. >>>> >>>> --- >>>> >>>> Michael Niedermayer wrote: >>>>> seems to break fate here: >>>> [...] >>>> >>>> Oops, seems I messed up running fate and missed the "warning: only a >>>> subset of the fate tests will be run because SAMPLES is not specified" >>>> warning it gave... Thanks for catching that. >>>> >>>> Seems this reverse fix is actually needed, as extradata is actually >>>> copied in the other direction (from st->internal->avctx to >>>> st->codecpar). >>> >>> it seems this causes some changes, quite possibly thats simply due to >>> less packets being read >>> >>> libavformat/tests/seek Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv -duration 400 >>> with http://samples.ffmpeg.org/Matroska/matrix/Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv >> >> @@ -9,7 +9,7 @@ >> ret: 0 st:13 flags:1 dts: 86.750000 pts: 86.750000 pos: -1 >> size: 50436 >> ret:-1 st: 1 flags:0 ts: 250.577000 >> ret: 0 st: 1 flags:1 ts: 13.471000 >> -ret: 0 st:13 flags:1 dts: 5.909000 pts: 5.909000 pos: -1 >> size: 50436 >> +ret: 0 st:13 flags:1 dts: 1.776000 pts: 1.776000 pos: -1 >> size: 50436 >> ret:-1 st: 2 flags:0 ts: 176.365000 >> ret: 0 st: 2 flags:1 ts: 339.259000 >> ret: 0 st:13 flags:1 dts: 145.080000 pts: 145.080000 pos: >> -1 size: 50436 >> >> Seems to indeed be due to less packets being read in >> avformat_find_stream_info(). >> >> Matroska demuxer seems to add keyframes to the index as it encounters >> them, and in this case more keyframes are encountered in >> avformat_stream_info() phase, which seems to result in more accurate >> seeking in the early seconds of the file. >> >>> for tickets//2254/ttvHD_vlc_sample.ts the tbr changes from >>> 1001/30000 to 1/30 >> >> Looks like the timestamps are jittery in that sample: >> >> 36072.231911, dts = prev + 3002 >> 36072.265289, dts = prev + 3004 >> 36072.298656, dts = prev + 3003 >> 36072.331433, dts = prev + 2950 >> 36072.364800, dts = prev + 3003 >> 36072.398167, dts = prev + 3003 >> 36072.431533, dts = prev + 3003 >> 36072.464900, dts = prev + 3003 >> 36072.498267, dts = prev + 3003 >> 36072.531633, dts = prev + 3003 >> 36072.565000, dts = prev + 3003 >> 36072.598367, dts = prev + 3003 >> 36072.630667, dts = prev + 2907 >> 36072.664033, dts = prev + 3003 >> 36072.697400, dts = prev + 3003 >> 36072.730767, dts = prev + 3003 >> 36072.764133, dts = prev + 3003 >> 36072.797500, dts = prev + 3003 >> 36072.830867, dts = prev + 3003 >> 36072.864233, dts = prev + 3003 >> 36072.897600, dts = prev + 3003 >> 36072.939278, dts = prev + 3751 >> 36072.972644, dts = prev + 3003 >> 36073.006011, dts = prev + 3003 >> 36073.034478, dts = prev + 2562 >> 36073.067844, dts = prev + 3003 >> >> And this throws the ff_rfps_calculate() algorithm off unless it is >> called with fpsprobesize (fps_analyze_framecount) of 160 or more >> (default is 20). >> >> Not sure if something could/should be done to keep tbr of that file to >> be detected as 1001/3000. >> >> >> For both of these cases I verified that FFmpeg 3.0 had the same behavior >> as with this patch, i.e. their behavior was unintendedly changed by the >> original commit 6f69f7a8bf6a0d01. > > thanks alot for the deep analysis > i have no objections to the change > > > [...] Applied.
diff --git a/libavformat/utils.c b/libavformat/utils.c index e5a99ff..5a902ea 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -3432,7 +3432,7 @@ FF_ENABLE_DEPRECATION_WARNINGS break; } if (st->parser && st->parser->parser->split && - !st->codecpar->extradata) + !st->internal->avctx->extradata) break; if (st->first_dts == AV_NOPTS_VALUE && !(ic->iformat->flags & AVFMT_NOTIMESTAMPS) &&
Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed avformat_find_stream_info() to put the extradata it got from st->parser->parser->split() to st->internal->avctx instead of st->codec (from where it will be later copied to st->codecpar). However, in the same function, the "is stream ready?" check was changed to check for extradata in st->codecpar instead of st->codec. Extradata retrieved from split() is therefore not considered anymore, and avformat_find_stream_info() will therefore needlessly continue probing in some cases. Fix that by checking for the extradata at st->internal->avctx where it is actually put. --- Michael Niedermayer wrote: > seems to break fate here: [...] Oops, seems I messed up running fate and missed the "warning: only a subset of the fate tests will be run because SAMPLES is not specified" warning it gave... Thanks for catching that. Seems this reverse fix is actually needed, as extradata is actually copied in the other direction (from st->internal->avctx to st->codecpar). Fate passes here now. libavformat/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)