Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
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.75 pts: 86.75 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.08 pts: 145.08 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/3 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.
--
Anssi Hannula
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
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.75 pts: 86.75 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.08 pts: 145.08 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/3 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
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.75 pts: 86.75 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.08 pts: 145.08 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/3 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
--
Anssi Hannula
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
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/3 to 1/30
are these in intended / expected ?
I can confirm that this reduces the amount of data read in many files
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Democracy is the form of government in which you can choose your dictator
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
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(-)
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) &&
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
On Tue, Jul 26, 2016 at 01:51:04PM +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 avctx instead of st->codec.
>
> 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 putting the extradata in st->codecpar, from where it will be
> copied to avctx at the end of avformat_find_stream_info() along with
> all other parameters.
>
> ---
>
> I'm not overly familiar with this code, so review accordingly :)
>
> This can be reproduced e.g. with a simple sample generated with
> "ffmpeg -t 20 -f lavfi -i cellauto out.ts", where ffprobe will analyze
> for 7 seconds (mpegts max_stream_analyze_duration) instead of the
> expected 5 seconds (max_analyze_duration).
>
> The difference will be more dramatic with streams that trigger the
> "stop find_stream_info from waiting for more streams when all programs
> have received a PMT" check in mpegts demuxer.
seems to break fate here:
make: *** [fate-iv8-demux] Error 1
make: *** [fate-lmlm4-demux] Error 1
make: *** [fate-nc-demux] Error 1
make: *** [fate-pva-demux] Error 69
make: *** [fate-wtv-demux] Error 1
make: *** [fate-ts-demux] Error 1
make: *** [fate-vc1_sa00040] Error 1
make: *** [fate-vc1_sa00050] Error 1
make: *** [fate-vc1_sa10091] Error 1
make: *** [fate-vc1_sa20021] Error 1
make: *** [fate-vc1_sa10143] Error 1
make: *** [fate-vc1_ilaced_twomv] Error
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
What does censorship reveal? It reveals fear. -- Julian Assange
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
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 avctx instead of st->codec.
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 putting the extradata in st->codecpar, from where it will be
copied to avctx at the end of avformat_find_stream_info() along with
all other parameters.
---
I'm not overly familiar with this code, so review accordingly :)
This can be reproduced e.g. with a simple sample generated with
"ffmpeg -t 20 -f lavfi -i cellauto out.ts", where ffprobe will analyze
for 7 seconds (mpegts max_stream_analyze_duration) instead of the
expected 5 seconds (max_analyze_duration).
The difference will be more dramatic with streams that trigger the
"stop find_stream_info from waiting for more streams when all programs
have received a PMT" check in mpegts demuxer.
libavformat/utils.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/libavformat/utils.c b/libavformat/utils.c
index e5a99ff..392143e 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -3581,16 +3581,16 @@ FF_ENABLE_DEPRECATION_WARNINGS
if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO)
ff_rfps_add_frame(ic, st, pkt->dts);
#endif
-if (st->parser && st->parser->parser->split && !avctx->extradata) {
+if (st->parser && st->parser->parser->split &&
!st->codecpar->extradata) {
int i = st->parser->parser->split(avctx, pkt->data, pkt->size);
if (i > 0 && i < FF_MAX_EXTRADATA_SIZE) {
-avctx->extradata_size = i;
-avctx->extradata = av_mallocz(avctx->extradata_size +
-
AV_INPUT_BUFFER_PADDING_SIZE);
-if (!avctx->extradata)
+st->codecpar->extradata_size = i;
+st->codecpar->extradata =
av_mallocz(st->codecpar->extradata_size +
+
AV_INPUT_BUFFER_PADDING_SIZE);
+if (!st->codecpar->extradata)
return AVERROR(ENOMEM);
-memcpy(avctx->extradata, pkt->data,
- avctx->extradata_size);
+memcpy(st->codecpar->extradata, pkt->data,
+ st->codecpar->extradata_size);
}
}
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
