[PATCH] lavf: inspect more frames when the time base is unreliable
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base. --- This seems the most straight-forward and safe fix for this issue, with the unfortunate cost of the 10 additional frames that have to be demuxed. Another (or additional) option could be to improve the heuristics themselves, if someone more experienced with this timing stuff wants to give it a try (or to provide hints). I guess one such way would be to fall back to codec time base early if it looks accurate based on the first few timestamps, but indeed I have no idea if it would break something else. Input welcome. For the record, here you can see the progression of the cumulative error of the two competing rates when detecting the frame rate: 0.042000 for 24.000000, at 0.042000, error 0.000333, cumulative 0.000000111 for 23.976024, at 0.042000, error 0.000292, cumulative 0.000000085 0.041000 for 24.000000, at 0.041000, error -0.000667, cumulative 0.000000444 for 23.976024, at 0.041000, error -0.000708, cumulative 0.000000502 0.083000 for 24.000000, at 0.083000, error -0.000333, cumulative 0.000000556 for 23.976024, at 0.083000, error -0.000417, cumulative 0.000000675 0.125000 for 24.000000, at 0.125000, error 0.000000, cumulative 0.000000556 for 23.976024, at 0.125000, error -0.000125, cumulative 0.000000691 0.167000 for 24.000000, at 0.167000, error 0.000333, cumulative 0.000000667 for 23.976024, at 0.167000, error 0.000167, cumulative 0.000000719 0.208000 for 24.000000, at 0.208000, error -0.000333, cumulative 0.000000778 for 23.976024, at 0.208000, error -0.000542, cumulative 0.000001012 0.250000 for 24.000000, at 0.250000, error 0.000000, cumulative 0.000000778 for 23.976024, at 0.250000, error -0.000250, cumulative 0.000001075 0.292000 for 24.000000, at 0.292000, error 0.000333, cumulative 0.000000889 for 23.976024, at 0.292000, error 0.000042, cumulative 0.000001076 0.333000 for 24.000000, at 0.333000, error -0.000333, cumulative 0.000001000 for 23.976024, at 0.333000, error -0.000667, cumulative 0.000001521 0.375000 for 24.000000, at 0.375000, error 0.000000, cumulative 0.000001000 for 23.976024, at 0.375000, error -0.000375, cumulative 0.000001661 0.417000 for 24.000000, at 0.417000, error 0.000333, cumulative 0.000001111 for 23.976024, at 0.417000, error -0.000083, cumulative 0.000001668 0.459000 for 24.000000, at 0.459000, error 0.000667, cumulative 0.000001556 for 23.976024, at 0.459000, error 0.000208, cumulative 0.000001712 0.500000 for 24.000000, at 0.500000, error 0.000000, cumulative 0.000001556 for 23.976024, at 0.500000, error -0.000500, cumulative 0.000001962 0.542000 for 24.000000, at 0.542000, error 0.000333, cumulative 0.000001667 for 23.976024, at 0.542000, error -0.000208, cumulative 0.000002005 0.584000 for 24.000000, at 0.584000, error 0.000667, cumulative 0.000002111 for 23.976024, at 0.584000, error 0.000083, cumulative 0.000002012 0.625000 for 24.000000, at 0.625000, error 0.000000, cumulative 0.000002111 for 23.976024, at 0.625000, error -0.000625, cumulative 0.000002403 0.667000 for 24.000000, at 0.667000, error 0.000333, cumulative 0.000002222 for 23.976024, at 0.667000, error -0.000333, cumulative 0.000002514 0.709000 for 24.000000, at 0.709000, error 0.000667, cumulative 0.000002667 for 23.976024, at 0.709000, error -0.000042, cumulative 0.000002516 0.750000 for 24.000000, at 0.750000, error 0.000000, cumulative 0.000002667 for 23.976024, at 0.750000, error -0.000750, cumulative 0.000003078 0.792000 for 24.000000, at 0.792000, error 0.000333, cumulative 0.000002778 for 23.976024, at 0.792000, error -0.000458, cumulative 0.000003288 Without the patch the detection stops at this point. Error for 24.000 is still smaller and it is wrongly selected. Raising the frame count to 30 allows the cumulative error for 24.000 to raise significantly when compared to 23.976, therefore ensuring the correct selection: 0.834000 for 24.000000, at 0.834000, error 0.000667, cumulative 0.000003222 for 23.976024, at 0.834000, error -0.000167, cumulative 0.000003316 0.876000 for 24.000000, at 0.876000, error 0.001000, cumulative 0.000004222 for 23.976024, at 0.876000, error 0.000125, cumulative 0.000003332 0.917000 for 24.000000, at 0.917000, error 0.000333, cumulative 0.000004333 for 23.976024, at 0.917000, error -0.000583, cumulative 0.000003672 0.959000 for 24.000000, at 0.959000, error 0.000667, cumulative 0.000004778 for 23.976024, at 0.959000, error -0.000292, cumulative 0.000003757 1.001000 for 24.000000, at 1.001000, error 0.001000, cumulative 0.000005778 for 23.976024, at 1.001000, error 0.000000, cumulative 0.000003757 1.042000 for 24.000000, at 1.042000, error 0.000333, cumulative 0.000005889 for 23.976024, at 1.042000, error -0.000708, cumulative 0.000004259 1.084000 for 24.000000, at 1.084000, error 0.000667, cumulative 0.000006333 for 23.976024, at 1.084000, error -0.000417, cumulative 0.000004432 1.126000 for 24.000000, at 1.126000, error 0.001000, cumulative 0.000007333 for 23.976024, at 1.126000, error -0.000125, cumulative 0.000004448 1.168000 for 24.000000, at 1.168000, error 0.001333, cumulative 0.000009111 for 23.976024, at 1.168000, error 0.000167, cumulative 0.000004476 1.209000 for 24.000000, at 1.209000, error 0.000667, cumulative 0.000009556 for 23.976024, at 1.209000, error -0.000542, cumulative 0.000004769 libavformat/utils.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index 4f51c26..8227c91 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2257,7 +2257,7 @@ int av_find_stream_info(AVFormatContext *ic) break; /* variable fps and no guess at the real fps */ if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) - && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) + && st->info->duration_count<30 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) break; -- 1.7.3
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist. -- M?ns Rullg?rd mans at mansr.com
On 01/29/2011 05:45 AM, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist.
Might be worth look at the file and see why that happens. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 29.01.2011 06:45, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist.
Yes, the timing information is correct in the headers. The timing estimation code is triggered simply because codec_id == CODEC_ID_H264, and the timings simply can't be estimated correctly in just 20 frames by the current algorithm with only millisecond-precision timestamps. -- Anssi Hannula
Anssi Hannula <anssi.hannula at iki.fi> writes:
On 29.01.2011 06:45, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist.
Yes, the timing information is correct in the headers.
The timing estimation code is triggered simply because codec_id == CODEC_ID_H264,
So remove that ugly hack, and all will be fine.
and the timings simply can't be estimated correctly in just 20 frames by the current algorithm with only millisecond-precision timestamps.
Nor should they. The frame duration from the header should be used. -- M?ns Rullg?rd mans at mansr.com
On Sat, Jan 29, 2011 at 01:41:15PM +0000, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
On 29.01.2011 06:45, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist.
Yes, the timing information is correct in the headers.
The timing estimation code is triggered simply because codec_id == CODEC_ID_H264,
So remove that ugly hack, and all will be fine.
I expect that to break PAFF.
On 29.01.2011 15:41, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
On 29.01.2011 06:45, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist.
Yes, the timing information is correct in the headers.
The timing estimation code is triggered simply because codec_id == CODEC_ID_H264,
So remove that ugly hack, and all will be fine.
Isn't the hack there for an actual reason? For the record, here's the commit that added it: http://git.ffmpeg.org/?p=ffmpeg.git;a=commit;h=7f123e7f8ad2cfd4ab7e2fbd83e17...
and the timings simply can't be estimated correctly in just 20 frames by the current algorithm with only millisecond-precision timestamps.
Nor should they. The frame duration from the header should be used.
Well, I see that for normal non-MPEG2/non-H264 files we use the framerate calculated from the codec time base (as below). Do you think using that would be ok for H.264 as well (at least if the first 5 or so timestamps seem to match that value, assuming H264 is considered unreliable for a reason), or if we should add some new case for using the format headers instead. For the files in question the codec time base shows an fps of 24/1.001 exactly.
if (!st->r_frame_rate.num){ if( st->codec->time_base.den * (int64_t)st->time_base.num <= st->codec->time_base.num * st->codec->ticks_per_frame * (int64_t)st->time_base.den){ st->r_frame_rate.num = st->codec->time_base.den; st->r_frame_rate.den = st->codec->time_base.num * st->codec->ticks_per_frame; }else{ st->r_frame_rate.num = st->time_base.den; st->r_frame_rate.den = st->time_base.num; } }
-- Anssi Hannula
On Sat, Jan 29, 2011 at 04:10:38PM +0200, Anssi Hannula wrote:
Well, I see that for normal non-MPEG2/non-H264 files we use the framerate calculated from the codec time base (as below). Do you think using that would be ok for H.264 as well (at least if the first 5 or so timestamps seem to match that value, assuming H264 is considered unreliable for a reason), or if we should add some new case for using the format headers instead.
While the code gets progressively uglier that way, using the value from the header if it's within a few % should be safe and fix at least that case.
On 29.01.2011 16:33, Reimar D?ffinger wrote:
On Sat, Jan 29, 2011 at 04:10:38PM +0200, Anssi Hannula wrote:
Well, I see that for normal non-MPEG2/non-H264 files we use the framerate calculated from the codec time base (as below). Do you think using that would be ok for H.264 as well (at least if the first 5 or so timestamps seem to match that value, assuming H264 is considered unreliable for a reason), or if we should add some new case for using the format headers instead.
While the code gets progressively uglier that way, using the value from the header if it's within a few % should be safe and fix at least that case.
Just to check, you mean the container header and not the h.264 stream header? If so, we would only use container header if tb_unreliable() is true and the first few timestamps match it, right? However, in this case the h.264 header has the correct fps as well, though, so wouldn't it be better if we used that if it seems to match, so that it would work for non-mkv as well? For the record, the value derived from h.264 header is 24000/1001 (~23.976024), while a value derived from the container header is 1710843747/71356439 (~23.976025) (interestingly, mkv header has 41708332ns while 24000/1001 would be 41708333.33...ns). I guess the difference is small enough, though. -- Anssi Hannula
Anssi Hannula <anssi.hannula at iki.fi> writes:
On 29.01.2011 15:41, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
On 29.01.2011 06:45, M?ns Rullg?rd wrote:
Anssi Hannula <anssi.hannula at iki.fi> writes:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
In my experience most matroska files have correct timing information in the headers. It sounds me more like the "clever" estimation code is hallucinating inaccuracies where none exist.
Yes, the timing information is correct in the headers.
The timing estimation code is triggered simply because codec_id == CODEC_ID_H264,
So remove that ugly hack, and all will be fine.
Isn't the hack there for an actual reason?
For the record, here's the commit that added it: http://git.ffmpeg.org/?p=ffmpeg.git;a=commit;h=7f123e7f8ad2cfd4ab7e2fbd83e17...
That commit message isn't exactly informative, though it hints at something to do with telecine. Telecined video can be easily detected by parsing a couple of frame headers looking for a repeat first field flag. There is no need to play guessing games with timestamps.
and the timings simply can't be estimated correctly in just 20 frames by the current algorithm with only millisecond-precision timestamps.
Nor should they. The frame duration from the header should be used.
Well, I see that for normal non-MPEG2/non-H264 files we use the framerate calculated from the codec time base (as below). Do you think using that would be ok for H.264 as well (at least if the first 5 or so timestamps seem to match that value, assuming H264 is considered unreliable for a reason), or if we should add some new case for using the format headers instead.
For the files in question the codec time base shows an fps of 24/1.001 exactly.
So the container header is correct, the elementary stream header is correct, and lavf still manages to arrive at a wrong value. Well done! -- M?ns Rullg?rd mans at mansr.com
On Sat, Jan 29, 2011 at 02:35:02PM +0000, M?ns Rullg?rd wrote: [...]
and the timings simply can't be estimated correctly in just 20 frames by the current algorithm with only millisecond-precision timestamps.
Nor should they. The frame duration from the header should be used.
Well, I see that for normal non-MPEG2/non-H264 files we use the framerate calculated from the codec time base (as below). Do you think using that would be ok for H.264 as well (at least if the first 5 or so timestamps seem to match that value, assuming H264 is considered unreliable for a reason), or if we should add some new case for using the format headers instead.
For the files in question the codec time base shows an fps of 24/1.001 exactly.
So the container header is correct, the elementary stream header is correct, and lavf still manages to arrive at a wrong value. Well done!
improve it please without breaking more cases than are currently broken. then brag about it being shit [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 2 "100% positive feedback" - "All either got their money back or didnt complain" "Best seller ever, very honest" - "Seller refunded buyer after failed scam"
On Sat, Jan 29, 2011 at 02:39:38AM +0200, Anssi Hannula wrote:
For the record, here you can see the progression of the cumulative error of the two competing rates when detecting the frame rate:
0.042000 for 24.000000, at 0.042000, error 0.000333, cumulative 0.000000111 for 23.976024, at 0.042000, error 0.000292, cumulative 0.000000085 0.041000 for 24.000000, at 0.041000, error -0.000667, cumulative 0.000000444 for 23.976024, at 0.041000, error -0.000708, cumulative 0.000000502 0.083000 for 24.000000, at 0.083000, error -0.000333, cumulative 0.000000556 for 23.976024, at 0.083000, error -0.000417, cumulative 0.000000675 0.125000 for 24.000000, at 0.125000, error 0.000000, cumulative 0.000000556 for 23.976024, at 0.125000, error -0.000125, cumulative 0.000000691 0.167000 for 24.000000, at 0.167000, error 0.000333, cumulative 0.000000667 for 23.976024, at 0.167000, error 0.000167, cumulative 0.000000719 0.208000 for 24.000000, at 0.208000, error -0.000333, cumulative 0.000000778 for 23.976024, at 0.208000, error -0.000542, cumulative 0.000001012 0.250000 for 24.000000, at 0.250000, error 0.000000, cumulative 0.000000778 for 23.976024, at 0.250000, error -0.000250, cumulative 0.000001075 0.292000 for 24.000000, at 0.292000, error 0.000333, cumulative 0.000000889 for 23.976024, at 0.292000, error 0.000042, cumulative 0.000001076 0.333000
Well, the problem is that the time stamps _do_ match 24 fps better than 23.97..., and it is pure chance that parsing more changes this. So this file simply isn't constant frame-rate. But it might be possibly to go for whatever is stored in the MKV file (if anything) in such an unclear case.
On 29.01.2011 12:28, Reimar D?ffinger wrote:
On Sat, Jan 29, 2011 at 02:39:38AM +0200, Anssi Hannula wrote:
For the record, here you can see the progression of the cumulative error of the two competing rates when detecting the frame rate:
0.042000 for 24.000000, at 0.042000, error 0.000333, cumulative 0.000000111 for 23.976024, at 0.042000, error 0.000292, cumulative 0.000000085 0.041000 for 24.000000, at 0.041000, error -0.000667, cumulative 0.000000444 for 23.976024, at 0.041000, error -0.000708, cumulative 0.000000502 0.083000 for 24.000000, at 0.083000, error -0.000333, cumulative 0.000000556 for 23.976024, at 0.083000, error -0.000417, cumulative 0.000000675 0.125000 for 24.000000, at 0.125000, error 0.000000, cumulative 0.000000556 for 23.976024, at 0.125000, error -0.000125, cumulative 0.000000691 0.167000 for 24.000000, at 0.167000, error 0.000333, cumulative 0.000000667 for 23.976024, at 0.167000, error 0.000167, cumulative 0.000000719 0.208000 for 24.000000, at 0.208000, error -0.000333, cumulative 0.000000778 for 23.976024, at 0.208000, error -0.000542, cumulative 0.000001012 0.250000 for 24.000000, at 0.250000, error 0.000000, cumulative 0.000000778 for 23.976024, at 0.250000, error -0.000250, cumulative 0.000001075 0.292000 for 24.000000, at 0.292000, error 0.000333, cumulative 0.000000889 for 23.976024, at 0.292000, error 0.000042, cumulative 0.000001076 0.333000
Well, the problem is that the time stamps _do_ match 24 fps better than 23.97..., and it is pure chance that parsing more changes this.
I don't think so. The timestamps have millisecond precision and they thus have a pattern of 41ms and 42ms differences. After 24 frames the timestamp is 1.001 seconds, which exactly matches 23.976 fps, not 24 fps (it would be 1.000 then). This is simply not detected with fewer frames due to the pattern being so long.
So this file simply isn't constant frame-rate.
The framerate is constant, but the timestamp differences are not (as they follow a pattern). Note that all 23.976 mkv files I've checked are like this.
But it might be possibly to go for whatever is stored in the MKV file (if anything) in such an unclear case.
The mkv file contains a default frame duration of 0.041708332ms, which is an approximation of 24000/1001 (23.976fps). -- Anssi Hannula
On Sat, Jan 29, 2011 at 02:35:04PM +0200, Anssi Hannula wrote:
On 29.01.2011 12:28, Reimar D?ffinger wrote:
On Sat, Jan 29, 2011 at 02:39:38AM +0200, Anssi Hannula wrote:
For the record, here you can see the progression of the cumulative error of the two competing rates when detecting the frame rate:
0.042000 for 24.000000, at 0.042000, error 0.000333, cumulative 0.000000111 for 23.976024, at 0.042000, error 0.000292, cumulative 0.000000085 0.041000 for 24.000000, at 0.041000, error -0.000667, cumulative 0.000000444 for 23.976024, at 0.041000, error -0.000708, cumulative 0.000000502 0.083000 for 24.000000, at 0.083000, error -0.000333, cumulative 0.000000556 for 23.976024, at 0.083000, error -0.000417, cumulative 0.000000675 0.125000 for 24.000000, at 0.125000, error 0.000000, cumulative 0.000000556 for 23.976024, at 0.125000, error -0.000125, cumulative 0.000000691 0.167000 for 24.000000, at 0.167000, error 0.000333, cumulative 0.000000667 for 23.976024, at 0.167000, error 0.000167, cumulative 0.000000719 0.208000 for 24.000000, at 0.208000, error -0.000333, cumulative 0.000000778 for 23.976024, at 0.208000, error -0.000542, cumulative 0.000001012 0.250000 for 24.000000, at 0.250000, error 0.000000, cumulative 0.000000778 for 23.976024, at 0.250000, error -0.000250, cumulative 0.000001075 0.292000 for 24.000000, at 0.292000, error 0.000333, cumulative 0.000000889 for 23.976024, at 0.292000, error 0.000042, cumulative 0.000001076 0.333000
Well, the problem is that the time stamps _do_ match 24 fps better than 23.97..., and it is pure chance that parsing more changes this.
I don't think so. The timestamps have millisecond precision and they thus have a pattern of 41ms and 42ms differences.
Sorry, failure to use calculator properly. What is the time_base, is FFmpeg aware the time stamps only have millisecond precision? Because I thought there was code to ignore only off-by-one differences.
But it might be possibly to go for whatever is stored in the MKV file (if anything) in such an unclear case.
The mkv file contains a default frame duration of 0.041708332ms, which is an approximation of 24000/1001 (23.976fps).
That could/should probably be used in some way then. Note that I do not know if it might also be that 24000/1001 causes an issue, that is a non-standard frame-rate, whereas 24 fps is standard and I think some parts of FFmpeg prefer standard values.
Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
On Sat, Jan 29, 2011 at 02:35:04PM +0200, Anssi Hannula wrote:
On 29.01.2011 12:28, Reimar D?ffinger wrote:
On Sat, Jan 29, 2011 at 02:39:38AM +0200, Anssi Hannula wrote:
For the record, here you can see the progression of the cumulative error of the two competing rates when detecting the frame rate:
0.042000 for 24.000000, at 0.042000, error 0.000333, cumulative 0.000000111 for 23.976024, at 0.042000, error 0.000292, cumulative 0.000000085 0.041000 for 24.000000, at 0.041000, error -0.000667, cumulative 0.000000444 for 23.976024, at 0.041000, error -0.000708, cumulative 0.000000502 0.083000 for 24.000000, at 0.083000, error -0.000333, cumulative 0.000000556 for 23.976024, at 0.083000, error -0.000417, cumulative 0.000000675 0.125000 for 24.000000, at 0.125000, error 0.000000, cumulative 0.000000556 for 23.976024, at 0.125000, error -0.000125, cumulative 0.000000691 0.167000 for 24.000000, at 0.167000, error 0.000333, cumulative 0.000000667 for 23.976024, at 0.167000, error 0.000167, cumulative 0.000000719 0.208000 for 24.000000, at 0.208000, error -0.000333, cumulative 0.000000778 for 23.976024, at 0.208000, error -0.000542, cumulative 0.000001012 0.250000 for 24.000000, at 0.250000, error 0.000000, cumulative 0.000000778 for 23.976024, at 0.250000, error -0.000250, cumulative 0.000001075 0.292000 for 24.000000, at 0.292000, error 0.000333, cumulative 0.000000889 for 23.976024, at 0.292000, error 0.000042, cumulative 0.000001076 0.333000
Well, the problem is that the time stamps _do_ match 24 fps better than 23.97..., and it is pure chance that parsing more changes this.
I don't think so. The timestamps have millisecond precision and they thus have a pattern of 41ms and 42ms differences.
Sorry, failure to use calculator properly. What is the time_base, is FFmpeg aware the time stamps only have millisecond precision? Because I thought there was code to ignore only off-by-one differences.
But it might be possibly to go for whatever is stored in the MKV file (if anything) in such an unclear case.
The mkv file contains a default frame duration of 0.041708332ms, which is an approximation of 24000/1001 (23.976fps).
That could/should probably be used in some way then. Note that I do not know if it might also be that 24000/1001 causes an issue, that is a non-standard frame-rate, whereas 24 fps is standard and I think some parts of FFmpeg prefer standard values.
The track DefaultDuration is by definition the frame duration in nanoseconds. If this is set, there is no need to attempt guessing from timestamps. -- M?ns Rullg?rd mans at mansr.com
On Sat, Jan 29, 2011 at 02:39:38AM +0200, Anssi Hannula wrote:
23.976 fps H.264 Matroska files are usually detected as 24 fps (r_frame_rate) by av_find_stream_info(). Fix that by inspecting the timestamps of 30 frames instead of just 20 for video codecs with unreliable time base.
While i agree with others that the header values should be used if they all match and fit in, in absence of any header values this code still seems to need to be improved as well so i would suggest that the timebase be checked and if its to inaccurate to detect this difference at 20 but detect it at 30 frames than 30 frames should be used. (more generic solutions may be possible too) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope
If the first 4 frames contain timestamps that closely match the codec time base, accept the codec time base in tb_unreliable() even if the codec is blacklisted (currently MPEG-2 and H.264). --- Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it. I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions. What do you think? libavformat/avformat.h | 2 ++ libavformat/utils.c | 38 +++++++++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/libavformat/avformat.h b/libavformat/avformat.h index f9f9be5..67e0da9 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -627,6 +627,8 @@ typedef struct AVStream { int64_t last_dts; int64_t duration_gcd; int duration_count; + int codec_tb_matches_dts; + double codec_tb_dur_error; double duration_error[MAX_STD_TIMEBASES]; int64_t codec_info_duration; } *info; diff --git a/libavformat/utils.c b/libavformat/utils.c index 4f51c26..61633f9 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2174,15 +2174,20 @@ static int get_std_framerate(int i){ * MPEG-2 commonly misuses field repeat flags to store different framerates. * And there are "variable" fps files this needs to detect as well. */ -static int tb_unreliable(AVCodecContext *c){ - if( c->time_base.den >= 101L*c->time_base.num - || c->time_base.den < 5L*c->time_base.num -/* || c->codec_tag == AV_RL32("DIVX") - || c->codec_tag == AV_RL32("XVID")*/ - || c->codec_id == CODEC_ID_MPEG2VIDEO - || c->codec_id == CODEC_ID_H264 - ) +static int tb_unreliable(AVStream *st){ + if( st->codec->time_base.den >= 101L*st->codec->time_base.num + || st->codec->time_base.den < 5L*st->codec->time_base.num) return 1; + + if(st->info->codec_tb_matches_dts) + return 0; + + if(/* st->codec->codec_tag == AV_RL32("DIVX") + || st->codec->codec_tag == AV_RL32("XVID") ||*/ + st->codec->codec_id == CODEC_ID_MPEG2VIDEO + || st->codec->codec_id == CODEC_ID_H264) + return 1; + return 0; } @@ -2256,7 +2261,7 @@ int av_find_stream_info(AVFormatContext *ic) if (!has_codec_parameters(st->codec)) break; /* variable fps and no guess at the real fps */ - if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) + if( tb_unreliable(st) && !(st->r_frame_rate.num && st->avg_frame_rate.num) && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) @@ -2336,6 +2341,16 @@ int av_find_stream_info(AVFormatContext *ic) st->info->duration_error[i] += error*error; } st->info->duration_count++; + + if (st->info->duration_count <= 4) { + double codec_tb_rate = st->codec->time_base.den / (double)st->codec->time_base.num * st->codec->ticks_per_frame; + double error = dur - lrintf(dur * codec_tb_rate) / codec_tb_rate; + st->info->codec_tb_dur_error += error*error; + + if (st->info->duration_count == 4 && st->info->codec_tb_dur_error * codec_tb_rate < 0.0001) + st->info->codec_tb_matches_dts = 1; + } + // ignore the first 4 values, they might have some random jitter if (st->info->duration_count > 3) st->info->duration_gcd = av_gcd(st->info->duration_gcd, duration); @@ -2383,10 +2398,11 @@ int av_find_stream_info(AVFormatContext *ic) // the check for tb_unreliable() is not completely correct, since this is not about handling // a unreliable/inexact time base, but a time base that is finer than necessary, as e.g. // ipmovie.c produces. - if (tb_unreliable(st->codec) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num) + if (tb_unreliable(st) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num) av_reduce(&st->r_frame_rate.num, &st->r_frame_rate.den, st->time_base.den, st->time_base.num * st->info->duration_gcd, INT_MAX); + if (st->info->duration_count && !st->r_frame_rate.num - && tb_unreliable(st->codec) /*&& + && tb_unreliable(st) /*&& //FIXME we should not special-case MPEG-2, but this needs testing with non-MPEG-2 ... st->time_base.num*duration_sum[i]/st->info->duration_count*101LL > st->time_base.den*/){ int num = 0; -- 1.7.3
Hi, On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { ? ? ? ? int64_t last_dts; ? ? ? ? int64_t duration_gcd; ? ? ? ? int duration_count; + ? ? ? ?int codec_tb_matches_dts; + ? ? ? ?double codec_tb_dur_error; ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; ? ? ? ? int64_t codec_info_duration; ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions? Ronald
On Tue, Feb 01, 2011 at 11:10:12AM -0500, Ronald S. Bultje wrote:
Hi,
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { ? ? ? ? int64_t last_dts; ? ? ? ? int64_t duration_gcd; ? ? ? ? int duration_count; + ? ? ? ?int codec_tb_matches_dts; + ? ? ? ?double codec_tb_dur_error; ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; ? ? ? ? int64_t codec_info_duration; ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there.
Can you elaborate on that?
Can you locally allocate and free an array instead,
thats would just be messy and fragile
or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
you sound confused, do you actually understand the code you try to review? [...] -- 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
On 01.02.2011 18:10, Ronald S. Bultje wrote:
Hi,
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { int64_t last_dts; int64_t duration_gcd; int duration_count; + int codec_tb_matches_dts; + double codec_tb_dur_error; double duration_error[MAX_STD_TIMEBASES]; int64_t codec_info_duration; } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
They are not directly in AVStream and do not increase the size of AVStream nor change the API. Here's a longer excerpt from the above section (before the patch): /** * Stream informations used internally by av_find_stream_info() */ #define MAX_STD_TIMEBASES (60*12+5) struct { int64_t last_dts; int64_t duration_gcd; int duration_count; double duration_error[MAX_STD_TIMEBASES]; int64_t codec_info_duration; } *info; -- Anssi Hannula
Hi, On Tue, Feb 1, 2011 at 11:49 AM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
On 01.02.2011 18:10, Ronald S. Bultje wrote:
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { ? ? ? ? int64_t last_dts; ? ? ? ? int64_t duration_gcd; ? ? ? ? int duration_count; + ? ? ? ?int codec_tb_matches_dts; + ? ? ? ?double codec_tb_dur_error; ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; ? ? ? ? int64_t codec_info_duration; ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
They are not directly in AVStream and do not increase the size of AVStream nor change the API.
Here's a longer excerpt from the above section (before the patch):
? ?/** ? ? * Stream informations used internally by av_find_stream_info() ? ? */ #define MAX_STD_TIMEBASES (60*12+5) ? ?struct { ? ? ? ?int64_t last_dts; ? ? ? ?int64_t duration_gcd; ? ? ? ?int duration_count; ? ? ? ?double duration_error[MAX_STD_TIMEBASES]; ? ? ? ?int64_t codec_info_duration; ? ?} *info;
Oh right it's a pointer. I still consider it ugly, ohwell if that's how it works now then I guess it's OK. Ronald
On Tue, Feb 01, 2011 at 12:15:34PM -0500, Ronald S. Bultje wrote:
Hi,
On Tue, Feb 1, 2011 at 11:49 AM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
On 01.02.2011 18:10, Ronald S. Bultje wrote:
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { ? ? ? ? int64_t last_dts; ? ? ? ? int64_t duration_gcd; ? ? ? ? int duration_count; + ? ? ? ?int codec_tb_matches_dts; + ? ? ? ?double codec_tb_dur_error; ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; ? ? ? ? int64_t codec_info_duration; ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
They are not directly in AVStream and do not increase the size of AVStream nor change the API.
Here's a longer excerpt from the above section (before the patch):
? ?/** ? ? * Stream informations used internally by av_find_stream_info() ? ? */ #define MAX_STD_TIMEBASES (60*12+5) ? ?struct { ? ? ? ?int64_t last_dts; ? ? ? ?int64_t duration_gcd; ? ? ? ?int duration_count; ? ? ? ?double duration_error[MAX_STD_TIMEBASES]; ? ? ? ?int64_t codec_info_duration; ? ?} *info;
Oh right it's a pointer. I still consider it ugly, ohwell if that's how it works now then I guess it's OK.
so how would you do it non ugly? This kind of comments remind me of mplayer, people also said "ugg ugly" then fixed it to be pretty but it stoped working bcause the ugly was actually the cleanest way that worked people just didnt understand it [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle
Hi, On Tue, Feb 1, 2011 at 12:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Feb 01, 2011 at 12:15:34PM -0500, Ronald S. Bultje wrote:
Hi,
On Tue, Feb 1, 2011 at 11:49 AM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
On 01.02.2011 18:10, Ronald S. Bultje wrote:
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { ? ? ? ? int64_t last_dts; ? ? ? ? int64_t duration_gcd; ? ? ? ? int duration_count; + ? ? ? ?int codec_tb_matches_dts; + ? ? ? ?double codec_tb_dur_error; ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; ? ? ? ? int64_t codec_info_duration; ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
They are not directly in AVStream and do not increase the size of AVStream nor change the API.
Here's a longer excerpt from the above section (before the patch):
? ?/** ? ? * Stream informations used internally by av_find_stream_info() ? ? */ #define MAX_STD_TIMEBASES (60*12+5) ? ?struct { ? ? ? ?int64_t last_dts; ? ? ? ?int64_t duration_gcd; ? ? ? ?int duration_count; ? ? ? ?double duration_error[MAX_STD_TIMEBASES]; ? ? ? ?int64_t codec_info_duration; ? ?} *info;
Oh right it's a pointer. I still consider it ugly, ohwell if that's how it works now then I guess it's OK.
so how would you do it non ugly?
This kind of comments remind me of mplayer, people also said "ugg ugly" then fixed it to be pretty but it stoped working bcause the ugly was actually the cleanest way that worked people just didnt understand it
I didn't say I had a non-ugly way, I just said it was ugly to put obviously private members in a publicly exposed struct, not even under a HAVE_AV_CONFIG_H (so that the thing is allocated internally at the right size, and anything at the end "appended" to the publicly exposed API is considered private/internal) or something alike. It's just asking for trouble. But like I said, it's fine, I won't fight this fight yet. Ronald
"Ronald S. Bultje" <rsbultje at gmail.com> writes:
Hi,
On Tue, Feb 1, 2011 at 12:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Feb 01, 2011 at 12:15:34PM -0500, Ronald S. Bultje wrote:
Hi,
On Tue, Feb 1, 2011 at 11:49 AM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
On 01.02.2011 18:10, Ronald S. Bultje wrote:
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
@@ -627,6 +627,8 @@ typedef struct AVStream { ? ? ? ? int64_t last_dts; ? ? ? ? int64_t duration_gcd; ? ? ? ? int duration_count; + ? ? ? ?int codec_tb_matches_dts; + ? ? ? ?double codec_tb_dur_error; ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; ? ? ? ? int64_t codec_info_duration; ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
They are not directly in AVStream and do not increase the size of AVStream nor change the API.
Here's a longer excerpt from the above section (before the patch):
? ?/** ? ? * Stream informations used internally by av_find_stream_info() ? ? */ #define MAX_STD_TIMEBASES (60*12+5) ? ?struct { ? ? ? ?int64_t last_dts; ? ? ? ?int64_t duration_gcd; ? ? ? ?int duration_count; ? ? ? ?double duration_error[MAX_STD_TIMEBASES]; ? ? ? ?int64_t codec_info_duration; ? ?} *info;
Oh right it's a pointer. I still consider it ugly, ohwell if that's how it works now then I guess it's OK.
so how would you do it non ugly?
This kind of comments remind me of mplayer, people also said "ugg ugly" then fixed it to be pretty but it stoped working bcause the ugly was actually the cleanest way that worked people just didnt understand it
I didn't say I had a non-ugly way, I just said it was ugly to put obviously private members in a publicly exposed struct, not even under a HAVE_AV_CONFIG_H (so that the thing is allocated internally at the right size, and anything at the end "appended" to the publicly exposed API is considered private/internal) or something alike.
It's just asking for trouble.
Define the struct in some private place and have just a "struct foo *" in the context. -- M?ns Rullg?rd mans at mansr.com
On Tue, Feb 01, 2011 at 06:24:45PM +0000, M?ns Rullg?rd wrote:
"Ronald S. Bultje" <rsbultje at gmail.com> writes:
Hi,
On Tue, Feb 1, 2011 at 12:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Feb 01, 2011 at 12:15:34PM -0500, Ronald S. Bultje wrote:
Hi,
On Tue, Feb 1, 2011 at 11:49 AM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
On 01.02.2011 18:10, Ronald S. Bultje wrote:
On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote: > @@ -627,6 +627,8 @@ typedef struct AVStream { > ? ? ? ? int64_t last_dts; > ? ? ? ? int64_t duration_gcd; > ? ? ? ? int duration_count; > + ? ? ? ?int codec_tb_matches_dts; > + ? ? ? ?double codec_tb_dur_error; > ? ? ? ? double duration_error[MAX_STD_TIMEBASES]; > ? ? ? ? int64_t codec_info_duration; > ? ? } *info;
These are only used in av_find_stream_info(), adding them to AVStream in the middle breaks ABI and increases size for something that I don't think should be in there. Can you locally allocate and free an array instead, or create a AVFindStreamInfo array for these kind of temp values to be shared between functions?
They are not directly in AVStream and do not increase the size of AVStream nor change the API.
Here's a longer excerpt from the above section (before the patch):
? ?/** ? ? * Stream informations used internally by av_find_stream_info() ? ? */ #define MAX_STD_TIMEBASES (60*12+5) ? ?struct { ? ? ? ?int64_t last_dts; ? ? ? ?int64_t duration_gcd; ? ? ? ?int duration_count; ? ? ? ?double duration_error[MAX_STD_TIMEBASES]; ? ? ? ?int64_t codec_info_duration; ? ?} *info;
Oh right it's a pointer. I still consider it ugly, ohwell if that's how it works now then I guess it's OK.
so how would you do it non ugly?
This kind of comments remind me of mplayer, people also said "ugg ugly" then fixed it to be pretty but it stoped working bcause the ugly was actually the cleanest way that worked people just didnt understand it
I didn't say I had a non-ugly way, I just said it was ugly to put obviously private members in a publicly exposed struct, not even under a HAVE_AV_CONFIG_H (so that the thing is allocated internally at the right size, and anything at the end "appended" to the publicly exposed API is considered private/internal) or something alike.
It's just asking for trouble.
Define the struct in some private place and have just a "struct foo *" in the context.
patch welcome [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato
If the first 4 frames contain timestamps that closely match the codec time base, accept the codec time base in tb_unreliable() even if the codec is blacklisted (currently MPEG-2 and H.264). --- Anssi Hannula wrote:
Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it.
I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions.
What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed. libavformat/avformat.h | 2 ++ libavformat/utils.c | 37 ++++++++++++++++++++++++++----------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 83289e4..cf3b8fd 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -627,6 +627,8 @@ typedef struct AVStream { int64_t last_dts; int64_t duration_gcd; int duration_count; + int codec_tb_matches_dts; + double codec_tb_dur_error; double duration_error[MAX_STD_TIMEBASES]; int64_t codec_info_duration; } *info; diff --git a/libavformat/utils.c b/libavformat/utils.c index d12bbc2..b044f4d 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2189,15 +2189,20 @@ static int get_std_framerate(int i){ * MPEG-2 commonly misuses field repeat flags to store different framerates. * And there are "variable" fps files this needs to detect as well. */ -static int tb_unreliable(AVCodecContext *c){ - if( c->time_base.den >= 101L*c->time_base.num - || c->time_base.den < 5L*c->time_base.num -/* || c->codec_tag == AV_RL32("DIVX") - || c->codec_tag == AV_RL32("XVID")*/ - || c->codec_id == CODEC_ID_MPEG2VIDEO - || c->codec_id == CODEC_ID_H264 - ) +static int tb_unreliable(AVStream *st){ + if( st->codec->time_base.den >= 101L*st->codec->time_base.num + || st->codec->time_base.den < 5L*st->codec->time_base.num) return 1; + + if(st->info->codec_tb_matches_dts) + return 0; + + if(/* st->codec->codec_tag == AV_RL32("DIVX") + || st->codec->codec_tag == AV_RL32("XVID") ||*/ + st->codec->codec_id == CODEC_ID_MPEG2VIDEO + || st->codec->codec_id == CODEC_ID_H264) + return 1; + return 0; } @@ -2271,7 +2276,7 @@ int av_find_stream_info(AVFormatContext *ic) if (!has_codec_parameters(st->codec)) break; /* variable fps and no guess at the real fps */ - if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) + if( tb_unreliable(st) && !(st->r_frame_rate.num && st->avg_frame_rate.num) && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) @@ -2351,6 +2356,16 @@ int av_find_stream_info(AVFormatContext *ic) st->info->duration_error[i] += error*error; } st->info->duration_count++; + + if (st->info->duration_count <= 4) { + double codec_tb_rate = st->codec->time_base.den / (double)st->codec->time_base.num * st->codec->ticks_per_frame; + double error = dur - lrintf(dur * codec_tb_rate) / codec_tb_rate; + st->info->codec_tb_dur_error += error*error; + + if (st->info->duration_count == 4 && st->info->codec_tb_dur_error * codec_tb_rate < 0.0001) + st->info->codec_tb_matches_dts = 1; + } + // ignore the first 4 values, they might have some random jitter if (st->info->duration_count > 3) st->info->duration_gcd = av_gcd(st->info->duration_gcd, duration); @@ -2398,10 +2413,10 @@ int av_find_stream_info(AVFormatContext *ic) // the check for tb_unreliable() is not completely correct, since this is not about handling // a unreliable/inexact time base, but a time base that is finer than necessary, as e.g. // ipmovie.c produces. - if (tb_unreliable(st->codec) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num) + if (tb_unreliable(st) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num) av_reduce(&st->r_frame_rate.num, &st->r_frame_rate.den, st->time_base.den, st->time_base.num * st->info->duration_gcd, INT_MAX); if (st->info->duration_count && !st->r_frame_rate.num - && tb_unreliable(st->codec) /*&& + && tb_unreliable(st) /*&& //FIXME we should not special-case MPEG-2, but this needs testing with non-MPEG-2 ... st->time_base.num*duration_sum[i]/st->info->duration_count*101LL > st->time_base.den*/){ int num = 0; -- 1.7.3
On 06.02.2011 08:32, Anssi Hannula wrote:
If the first 4 frames contain timestamps that closely match the codec time base, accept the codec time base in tb_unreliable() even if the codec is blacklisted (currently MPEG-2 and H.264).
---
Anssi Hannula wrote:
Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it.
I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions.
What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames. When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals). At least this sample shows the above issue: http://samples.ffmpeg.org/V-codecs/h264/hdtv-interlaced/sky_720p_test_why-ca... So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :) I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
libavformat/avformat.h | 2 ++ libavformat/utils.c | 37 ++++++++++++++++++++++++++----------- 2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 83289e4..cf3b8fd 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -627,6 +627,8 @@ typedef struct AVStream { int64_t last_dts; int64_t duration_gcd; int duration_count; + int codec_tb_matches_dts; + double codec_tb_dur_error; double duration_error[MAX_STD_TIMEBASES]; int64_t codec_info_duration; } *info; diff --git a/libavformat/utils.c b/libavformat/utils.c index d12bbc2..b044f4d 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2189,15 +2189,20 @@ static int get_std_framerate(int i){ * MPEG-2 commonly misuses field repeat flags to store different framerates. * And there are "variable" fps files this needs to detect as well. */ -static int tb_unreliable(AVCodecContext *c){ - if( c->time_base.den >= 101L*c->time_base.num - || c->time_base.den < 5L*c->time_base.num -/* || c->codec_tag == AV_RL32("DIVX") - || c->codec_tag == AV_RL32("XVID")*/ - || c->codec_id == CODEC_ID_MPEG2VIDEO - || c->codec_id == CODEC_ID_H264 - ) +static int tb_unreliable(AVStream *st){ + if( st->codec->time_base.den >= 101L*st->codec->time_base.num + || st->codec->time_base.den < 5L*st->codec->time_base.num) return 1; + + if(st->info->codec_tb_matches_dts) + return 0; + + if(/* st->codec->codec_tag == AV_RL32("DIVX") + || st->codec->codec_tag == AV_RL32("XVID") ||*/ + st->codec->codec_id == CODEC_ID_MPEG2VIDEO + || st->codec->codec_id == CODEC_ID_H264) + return 1; + return 0; }
@@ -2271,7 +2276,7 @@ int av_find_stream_info(AVFormatContext *ic) if (!has_codec_parameters(st->codec)) break; /* variable fps and no guess at the real fps */ - if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) + if( tb_unreliable(st) && !(st->r_frame_rate.num && st->avg_frame_rate.num) && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) @@ -2351,6 +2356,16 @@ int av_find_stream_info(AVFormatContext *ic) st->info->duration_error[i] += error*error; } st->info->duration_count++; + + if (st->info->duration_count <= 4) { + double codec_tb_rate = st->codec->time_base.den / (double)st->codec->time_base.num * st->codec->ticks_per_frame;
And the above should be "/ st->codec->ticks_per_frame;", I think (didn't look in detail right now).
+ double error = dur - lrintf(dur * codec_tb_rate) / codec_tb_rate; + st->info->codec_tb_dur_error += error*error; + + if (st->info->duration_count == 4 && st->info->codec_tb_dur_error * codec_tb_rate < 0.0001) + st->info->codec_tb_matches_dts = 1; + } + // ignore the first 4 values, they might have some random jitter if (st->info->duration_count > 3) st->info->duration_gcd = av_gcd(st->info->duration_gcd, duration); @@ -2398,10 +2413,10 @@ int av_find_stream_info(AVFormatContext *ic) // the check for tb_unreliable() is not completely correct, since this is not about handling // a unreliable/inexact time base, but a time base that is finer than necessary, as e.g. // ipmovie.c produces. - if (tb_unreliable(st->codec) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num) + if (tb_unreliable(st) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num) av_reduce(&st->r_frame_rate.num, &st->r_frame_rate.den, st->time_base.den, st->time_base.num * st->info->duration_gcd, INT_MAX); if (st->info->duration_count && !st->r_frame_rate.num - && tb_unreliable(st->codec) /*&& + && tb_unreliable(st) /*&& //FIXME we should not special-case MPEG-2, but this needs testing with non-MPEG-2 ... st->time_base.num*duration_sum[i]/st->info->duration_count*101LL > st->time_base.den*/){ int num = 0;
-- Anssi Hannula
On Sun, Feb 13, 2011 at 12:52:35PM +0200, Anssi Hannula wrote:
On 06.02.2011 08:32, Anssi Hannula wrote:
If the first 4 frames contain timestamps that closely match the codec time base, accept the codec time base in tb_unreliable() even if the codec is blacklisted (currently MPEG-2 and H.264).
---
Anssi Hannula wrote:
Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it.
I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions.
What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames.
When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals).
At least this sample shows the above issue: http://samples.ffmpeg.org/V-codecs/h264/hdtv-interlaced/sky_720p_test_why-ca...
So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :)
I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
Could you explain elaborately what issue you are trying to fix? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
On 14.02.2011 14:24, Michael Niedermayer wrote:
On Sun, Feb 13, 2011 at 12:52:35PM +0200, Anssi Hannula wrote:
On 06.02.2011 08:32, Anssi Hannula wrote:
Anssi Hannula wrote:
Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it.
I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions.
What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames.
When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals).
At least this sample shows the above issue:
So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :)
I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
Could you explain elaborately what issue you are trying to fix?
OK, here's a recap of this thread :) - normally lavf assumes that the (1 / codec->time_base / codec->ticks_per_frame) is the fps (r_frame_rate), unless it would be too high to be represented by the st->time_base, in which case (1 / st->time_base) is taken instead (utils.c 2431-2440) - if ((1/5) > codec->time_base >= (1/101)) is false, the codec->time_base is considered unreliable and instead a custom fps probing code is used (tb_unreliable()) which reads the timestamps of the first 20 packets - some H.264 (and I guess MPEG2) streams have a specific interlacing mode that causes there to be 2x more packets (as output from the demuxer) than codec->time_base and codec->ticks_per_frame would indicate (well, I guess technically they are correct, if the packets actually contain half-frames due to interlacing), e.g. [1] - due to the above, H264 and MPEG2 are always assumed to have unreliable timebase and the fps probing code is always used - mkv tracks have generally millisecond precision for timestamps - 23.976fps therefore requires a pattern of 41ms and 42ms frames, that add up to 1.001s in 24 frames - the fps probing code doesn't detect the difference between 24fps and 23.976fps from just 20 frames, it would need more than that (25-30) => 23.976fps files are wrongly detected as 24fps - 23.976fps files are progressive (unless insane), so the codec timebase as got from the decoder would actually be reliable and show an exact rate of 24/1.001. - mkv tracks also contain a default_duration field that contains the length of frames in nanosecond precision. In the file I checked it was accurate to within 1.5ns (some rounding issue I guess), which corresponds to an error of about 0.000001 fps. So the issue is that 23.976 h264 mkv files are detected as having wrong r_frame_rate of 24. [1] http://samples.ffmpeg.org/V-codecs/h264/hdtv-interlaced/sky_720p_test_why-ca... -- Anssi Hannula
On Mon, Feb 14, 2011 at 05:00:25PM +0200, Anssi Hannula wrote:
On 14.02.2011 14:24, Michael Niedermayer wrote:
On Sun, Feb 13, 2011 at 12:52:35PM +0200, Anssi Hannula wrote:
On 06.02.2011 08:32, Anssi Hannula wrote:
Anssi Hannula wrote:
Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it.
I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions.
What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames.
When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals).
At least this sample shows the above issue:
So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :)
I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
Could you explain elaborately what issue you are trying to fix?
OK, here's a recap of this thread :)
- normally lavf assumes that the (1 / codec->time_base / codec->ticks_per_frame) is the fps (r_frame_rate), unless it would be too high to be represented by the st->time_base, in which case (1 / st->time_base) is taken instead (utils.c 2431-2440)
- if ((1/5) > codec->time_base >= (1/101)) is false, the codec->time_base is considered unreliable and instead a custom fps probing code is used (tb_unreliable()) which reads the timestamps of the first 20 packets
- some H.264 (and I guess MPEG2) streams have a specific interlacing mode that causes there to be 2x more packets (as output from the demuxer) than codec->time_base and codec->ticks_per_frame would indicate (well, I guess technically they are correct, if the packets actually contain half-frames due to interlacing), e.g. [1]
- due to the above, H264 and MPEG2 are always assumed to have unreliable timebase and the fps probing code is always used
- mkv tracks have generally millisecond precision for timestamps
- 23.976fps therefore requires a pattern of 41ms and 42ms frames, that add up to 1.001s in 24 frames
- the fps probing code doesn't detect the difference between 24fps and 23.976fps from just 20 frames, it would need more than that (25-30) => 23.976fps files are wrongly detected as 24fps
- 23.976fps files are progressive (unless insane), so the codec timebase as got from the decoder would actually be reliable and show an exact rate of 24/1.001.
- mkv tracks also contain a default_duration field that contains the length of frames in nanosecond precision. In the file I checked it was accurate to within 1.5ns (some rounding issue I guess), which corresponds to an error of about 0.000001 fps.
So the issue is that 23.976 h264 mkv files are detected as having wrong r_frame_rate of 24.
i need a sample to look into this (and sorry if a url was posted and i missed it) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB You can kill me, but you cannot change the truth.
On 15.02.2011 01:54, Michael Niedermayer wrote:
On Mon, Feb 14, 2011 at 05:00:25PM +0200, Anssi Hannula wrote:
On 14.02.2011 14:24, Michael Niedermayer wrote:
On Sun, Feb 13, 2011 at 12:52:35PM +0200, Anssi Hannula wrote:
On 06.02.2011 08:32, Anssi Hannula wrote:
Anssi Hannula wrote:
Here's a new patch that checks the timestamps of the first 4 frames (using the same method which is used in the guess-framerate code) and uses codec time base for r_frame_rate if the timestamps fit to it.
I tested also with several other files, including H.264 PAFF, MVE (ipmovie.c), and spotted no regressions.
What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames.
When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals).
At least this sample shows the above issue:
So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :)
I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
Could you explain elaborately what issue you are trying to fix?
OK, here's a recap of this thread :)
- normally lavf assumes that the (1 / codec->time_base / codec->ticks_per_frame) is the fps (r_frame_rate), unless it would be too high to be represented by the st->time_base, in which case (1 / st->time_base) is taken instead (utils.c 2431-2440)
- if ((1/5) > codec->time_base >= (1/101)) is false, the codec->time_base is considered unreliable and instead a custom fps probing code is used (tb_unreliable()) which reads the timestamps of the first 20 packets
- some H.264 (and I guess MPEG2) streams have a specific interlacing mode that causes there to be 2x more packets (as output from the demuxer) than codec->time_base and codec->ticks_per_frame would indicate (well, I guess technically they are correct, if the packets actually contain half-frames due to interlacing), e.g. [1]
- due to the above, H264 and MPEG2 are always assumed to have unreliable timebase and the fps probing code is always used
- mkv tracks have generally millisecond precision for timestamps
- 23.976fps therefore requires a pattern of 41ms and 42ms frames, that add up to 1.001s in 24 frames
- the fps probing code doesn't detect the difference between 24fps and 23.976fps from just 20 frames, it would need more than that (25-30) => 23.976fps files are wrongly detected as 24fps
- 23.976fps files are progressive (unless insane), so the codec timebase as got from the decoder would actually be reliable and show an exact rate of 24/1.001.
- mkv tracks also contain a default_duration field that contains the length of frames in nanosecond precision. In the file I checked it was accurate to within 1.5ns (some rounding issue I guess), which corresponds to an error of about 0.000001 fps.
So the issue is that 23.976 h264 mkv files are detected as having wrong r_frame_rate of 24.
i need a sample to look into this (and sorry if a url was posted and i missed it)
Well, here's one: http://onse.fi/files/h264-23.976-detected-as-24.mkv -- Anssi Hannula
On 15.02.2011 07:40, Anssi Hannula wrote:
On 15.02.2011 01:54, Michael Niedermayer wrote:
On Mon, Feb 14, 2011 at 05:00:25PM +0200, Anssi Hannula wrote:
On 14.02.2011 14:24, Michael Niedermayer wrote:
On Sun, Feb 13, 2011 at 12:52:35PM +0200, Anssi Hannula wrote:
On 06.02.2011 08:32, Anssi Hannula wrote:
Anssi Hannula wrote: > Here's a new patch that checks the timestamps of the first 4 frames > (using the same method which is used in the guess-framerate code) and > uses codec time base for r_frame_rate if the timestamps fit to it. > > I tested also with several other files, including H.264 PAFF, MVE > (ipmovie.c), and spotted no regressions. > > What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames.
When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals).
At least this sample shows the above issue:
So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :)
I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
Could you explain elaborately what issue you are trying to fix?
OK, here's a recap of this thread :)
- normally lavf assumes that the (1 / codec->time_base / codec->ticks_per_frame) is the fps (r_frame_rate), unless it would be too high to be represented by the st->time_base, in which case (1 / st->time_base) is taken instead (utils.c 2431-2440)
- if ((1/5) > codec->time_base >= (1/101)) is false, the codec->time_base is considered unreliable and instead a custom fps probing code is used (tb_unreliable()) which reads the timestamps of the first 20 packets
- some H.264 (and I guess MPEG2) streams have a specific interlacing mode that causes there to be 2x more packets (as output from the demuxer) than codec->time_base and codec->ticks_per_frame would indicate (well, I guess technically they are correct, if the packets actually contain half-frames due to interlacing), e.g. [1]
- due to the above, H264 and MPEG2 are always assumed to have unreliable timebase and the fps probing code is always used
- mkv tracks have generally millisecond precision for timestamps
- 23.976fps therefore requires a pattern of 41ms and 42ms frames, that add up to 1.001s in 24 frames
- the fps probing code doesn't detect the difference between 24fps and 23.976fps from just 20 frames, it would need more than that (25-30) => 23.976fps files are wrongly detected as 24fps
- 23.976fps files are progressive (unless insane), so the codec timebase as got from the decoder would actually be reliable and show an exact rate of 24/1.001.
- mkv tracks also contain a default_duration field that contains the length of frames in nanosecond precision. In the file I checked it was accurate to within 1.5ns (some rounding issue I guess), which corresponds to an error of about 0.000001 fps.
So the issue is that 23.976 h264 mkv files are detected as having wrong r_frame_rate of 24.
i need a sample to look into this (and sorry if a url was posted and i missed it)
Well, here's one: http://onse.fi/files/h264-23.976-detected-as-24.mkv
As there has been no activity, I added this to roundup to make sure it will not be forgotten: https://roundup.ffmpeg.org/issue2629 -- Anssi Hannula
On Tue, Feb 15, 2011 at 07:40:30AM +0200, Anssi Hannula wrote:
On 15.02.2011 01:54, Michael Niedermayer wrote:
On Mon, Feb 14, 2011 at 05:00:25PM +0200, Anssi Hannula wrote:
On 14.02.2011 14:24, Michael Niedermayer wrote:
On Sun, Feb 13, 2011 at 12:52:35PM +0200, Anssi Hannula wrote:
On 06.02.2011 08:32, Anssi Hannula wrote:
Anssi Hannula wrote: > Here's a new patch that checks the timestamps of the first 4 frames > (using the same method which is used in the guess-framerate code) and > uses codec time base for r_frame_rate if the timestamps fit to it. > > I tested also with several other files, including H.264 PAFF, MVE > (ipmovie.c), and spotted no regressions. > > What do you think?
Ping. Also, I noticed an extra added newline in the patch, now removed.
This patch is indeed not enough. The h264 decoder may not discover the time base immediately from the first few frames, so st->codec->time_base may still be stream time base. So the code will notice from the first 4 frames that the stream time base can accurately represent timestamps (which is always true), and sets codec_tb_matches_dts = 1. Since the mpegts timebase is 1/90000 (i.e. too fine to be fps), tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1, so the detection code continues to inspect frames.
When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is assumed correct due to codec_tb_matches_dts==1, even if it is not able accurately represent timestamps at all (e.g. 1/50 intervals).
At least this sample shows the above issue:
So some checks need to be added to the patch to guard against above. Or use some entirely different/better approach :)
I'd really appreciate someone more experienced looking at this issue, but I'll take a further look at this myself later as well.
Could you explain elaborately what issue you are trying to fix?
OK, here's a recap of this thread :)
- normally lavf assumes that the (1 / codec->time_base / codec->ticks_per_frame) is the fps (r_frame_rate), unless it would be too high to be represented by the st->time_base, in which case (1 / st->time_base) is taken instead (utils.c 2431-2440)
- if ((1/5) > codec->time_base >= (1/101)) is false, the codec->time_base is considered unreliable and instead a custom fps probing code is used (tb_unreliable()) which reads the timestamps of the first 20 packets
- some H.264 (and I guess MPEG2) streams have a specific interlacing mode that causes there to be 2x more packets (as output from the demuxer) than codec->time_base and codec->ticks_per_frame would indicate (well, I guess technically they are correct, if the packets actually contain half-frames due to interlacing), e.g. [1]
- due to the above, H264 and MPEG2 are always assumed to have unreliable timebase and the fps probing code is always used
- mkv tracks have generally millisecond precision for timestamps
- 23.976fps therefore requires a pattern of 41ms and 42ms frames, that add up to 1.001s in 24 frames
- the fps probing code doesn't detect the difference between 24fps and 23.976fps from just 20 frames, it would need more than that (25-30) => 23.976fps files are wrongly detected as 24fps
- 23.976fps files are progressive (unless insane), so the codec timebase as got from the decoder would actually be reliable and show an exact rate of 24/1.001.
- mkv tracks also contain a default_duration field that contains the length of frames in nanosecond precision. In the file I checked it was accurate to within 1.5ns (some rounding issue I guess), which corresponds to an error of about 0.000001 fps.
So the issue is that 23.976 h264 mkv files are detected as having wrong r_frame_rate of 24.
i need a sample to look into this (and sorry if a url was posted and i missed it)
Well, here's one: http://onse.fi/files/h264-23.976-detected-as-24.mkv
this has been fixed a while ago if you have other files that failed and still fail, iam interrsted in them [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
As per issue2629, most 23.976fps matroska H.264 files are incorrectly detected as 24fps, as the matroska timestamps usually have only millisecond precision. Fix that by doubling the amount of timestamps inspected for frame rate for streams that have coarse time base. This also fixes 29.970 detection in matroska. --- A preferable solution to issue2629 would be to somehow use header values either from the H.264 stream or from the Matroska stream. However, AFAICS the framerate inspecting has to be fixed as well anyway (with e.g. this patch), for cases when we have no reliable header values. Comments welcome. --- libavformat/utils.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index 1f0164f..0e73e52 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2273,12 +2273,20 @@ int av_find_stream_info(AVFormatContext *ic) /* check if one codec still needs to be handled */ for(i=0;i<ic->nb_streams;i++) { + int fps_analyze_framecount = 20; + st = ic->streams[i]; if (!has_codec_parameters(st->codec)) break; + /* if the timebase is coarse (like the usual millisecond precision + of mkv), we need to analyze more frames to reliably arrive at + the correct fps */ + if (av_q2d(st->time_base) > 0.0005) + fps_analyze_framecount *= 2; /* variable fps and no guess at the real fps */ if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) - && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) + && st->info->duration_count < fps_analyze_framecount + && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) break; -- 1.7.3
Ping? On Mon, Feb 28, 2011 at 02:20:56AM +0200, Anssi Hannula wrote:
As per issue2629, most 23.976fps matroska H.264 files are incorrectly detected as 24fps, as the matroska timestamps usually have only millisecond precision.
Fix that by doubling the amount of timestamps inspected for frame rate for streams that have coarse time base. This also fixes 29.970 detection in matroska.
---
A preferable solution to issue2629 would be to somehow use header values either from the H.264 stream or from the Matroska stream.
However, AFAICS the framerate inspecting has to be fixed as well anyway (with e.g. this patch), for cases when we have no reliable header values.
Comments welcome. --- libavformat/utils.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/libavformat/utils.c b/libavformat/utils.c index 1f0164f..0e73e52 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2273,12 +2273,20 @@ int av_find_stream_info(AVFormatContext *ic)
/* check if one codec still needs to be handled */ for(i=0;i<ic->nb_streams;i++) { + int fps_analyze_framecount = 20; + st = ic->streams[i]; if (!has_codec_parameters(st->codec)) break; + /* if the timebase is coarse (like the usual millisecond precision + of mkv), we need to analyze more frames to reliably arrive at + the correct fps */ + if (av_q2d(st->time_base) > 0.0005) + fps_analyze_framecount *= 2; /* variable fps and no guess at the real fps */ if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) - && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) + && st->info->duration_count < fps_analyze_framecount + && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) break; -- 1.7.3
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
On 27.03.2011 18:04, Reimar Döffinger wrote:
Ping?
Ping.
On Mon, Feb 28, 2011 at 02:20:56AM +0200, Anssi Hannula wrote:
As per issue2629, most 23.976fps matroska H.264 files are incorrectly detected as 24fps, as the matroska timestamps usually have only millisecond precision.
Fix that by doubling the amount of timestamps inspected for frame rate for streams that have coarse time base. This also fixes 29.970 detection in matroska.
---
A preferable solution to issue2629 would be to somehow use header values either from the H.264 stream or from the Matroska stream.
However, AFAICS the framerate inspecting has to be fixed as well anyway (with e.g. this patch), for cases when we have no reliable header values.
Comments welcome. --- libavformat/utils.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/libavformat/utils.c b/libavformat/utils.c index 1f0164f..0e73e52 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2273,12 +2273,20 @@ int av_find_stream_info(AVFormatContext *ic)
/* check if one codec still needs to be handled */ for(i=0;i<ic->nb_streams;i++) { + int fps_analyze_framecount = 20; + st = ic->streams[i]; if (!has_codec_parameters(st->codec)) break; + /* if the timebase is coarse (like the usual millisecond precision + of mkv), we need to analyze more frames to reliably arrive at + the correct fps */ + if (av_q2d(st->time_base) > 0.0005) + fps_analyze_framecount *= 2; /* variable fps and no guess at the real fps */ if( tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num) - && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) + && st->info->duration_count < fps_analyze_framecount + && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) break; if(st->parser && st->parser->parser->split && !st->codec->extradata) break; -- 1.7.3
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-- Anssi Hannula
On Mon, Feb 28, 2011 at 02:20:56AM +0200, Anssi Hannula wrote:
As per issue2629, most 23.976fps matroska H.264 files are incorrectly detected as 24fps, as the matroska timestamps usually have only millisecond precision.
Fix that by doubling the amount of timestamps inspected for frame rate for streams that have coarse time base. This also fixes 29.970 detection in matroska.
---
A preferable solution to issue2629 would be to somehow use header values either from the H.264 stream or from the Matroska stream.
However, AFAICS the framerate inspecting has to be fixed as well anyway (with e.g. this patch), for cases when we have no reliable header values.
Comments welcome.
applied locally, will push in a moment sorry, ive somehow missed reimars ping it seems [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
participants (9)
-
Anssi Hannula -
anssi.hannula＠iki.fi
-
lu_zero＠gentoo.org -
mans＠mansr.com -
Michael Niedermayer -
michaelni＠gmx.at
-
Reimar Döffinger -
Reimar.Doeffinger＠gmx.de
-
rsbultje＠gmail.com