Re: [libav-devel] [PATCH] make parsers' repeat_pict consistent with decoders
Hi, On 08/26/2011 12:55 PM, Vladimir Pantelic wrote: > On 08/26/2011 05:43 PM, John Stebbins wrote: >> Hi, >> >> On 08/26/2011 05:48 AM, Vladimir Pantelic wrote: > >>> int ticks = st->st->codec->ticks_per_frame + (ist->st->parser ? >>> ist->st->parser->repeat_pict : 0); >>> >> >> Well, after all that, I did some more code reading and spelunking of mailing >> list archives and discovered >> the reason that the code is the way it is. The decoders define repeat_pict >> == 0 to mean one frame time because >> a decoder always returns frames. The parser defines repeat_pict == 0 to >> mean one field time because >> the parser can return after parsing only a field. So when the parser sees a >> frame, it must add 1 >> to repeat pict so that the caller can distinguish field from frame. I think >> this is rather obtuse, but >> it is what it is. Is anybody in favor of adding a field to >> AVCodecParserContext to explicitly flag field >> vs. frame? This option was discussed and rejected in the original mailing >> list discussion. >> This was all discussed in the context of this thread back in feb 2009. >> >> http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/84007/focus=84344 >> > > then maybe at least add some comment lines, otherwise somebody else > will "discover" that again in 2013... > I'll throw together a patch that improves the comments in avcodec.h for now. Longer term, I'm investigating what it would take to do this right so it works correctly with h.264 VFR video. The code currently makes assumptions all over the place that ticks per field == 1 (when there can be fields) and that ticks_per_frame == ticks per field + 1. It should instead assume that ticks_per_frame can be anything and that ticks per field == ticks_per_frame / 2. I can do this using the current definitions of repeat_pict, or I can add a flag that explicitly signals when the parser is outputting a field. I prefer that latter since it removes the obfuscation and provides additional information that might actually be useful. But I would like some opinions before I continue with that. Changing those incorrect assumptions means code that does this: duration = time_base if (repeat_pict) duration *= (1 + repeat_pict) must become (if we keep the current definitions of repeat_pict) duration = (1 + repeat_pict) * time_base * ticks_per_frame / ((ticks_per_frame > 1) + 1) or if we used a field flag to designate fields vs frames: duration = (1 + !field + repeat_pict) * time_base * ticks_per_frame / ((ticks_per_frame > 1) + 1) This satisfies the case where there are fields and ticks_per_frame > 1. repeat_pict specifies the number of additional field times. And it satisfies the case where there are not fields and ticks_per_frame == 1. repeat_pict specifies the number of additional frame times. signature.asc Description: OpenPGP digital signature ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] make parsers' repeat_pict consistent with decoders
On 08/26/2011 05:43 PM, John Stebbins wrote: Hi, On 08/26/2011 05:48 AM, Vladimir Pantelic wrote: int ticks = st->st->codec->ticks_per_frame + (ist->st->parser ? ist->st->parser->repeat_pict : 0); Well, after all that, I did some more code reading and spelunking of mailing list archives and discovered the reason that the code is the way it is. The decoders define repeat_pict == 0 to mean one frame time because a decoder always returns frames. The parser defines repeat_pict == 0 to mean one field time because the parser can return after parsing only a field. So when the parser sees a frame, it must add 1 to repeat pict so that the caller can distinguish field from frame. I think this is rather obtuse, but it is what it is. Is anybody in favor of adding a field to AVCodecParserContext to explicitly flag field vs. frame? This option was discussed and rejected in the original mailing list discussion. This was all discussed in the context of this thread back in feb 2009. http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/84007/focus=84344 then maybe at least add some comment lines, otherwise somebody else will "discover" that again in 2013... ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] make parsers' repeat_pict consistent with decoders
Hi,
On 08/26/2011 05:48 AM, Vladimir Pantelic wrote:
> John Stebbins wrote:
>> On 08/25/2011 07:45 PM, John Stebbins wrote:
>>> As we discussed on IRC this morning, this makes the value of repeat_pict
>>> returned
>>> by the parsers the same as the value returned by the decoders.
>>>
>>> The parser was returning a value for repeat_pict that was the
>>> actual number of repeats + 1.
>>>
>>> Passes fate with the exception of 1 pre-existing failure
>>> (fate-h264-conformance-mr3_tandberg_b).
>>
>> Heh, fate passed, but some samples I have that use pulldown didn't play
>> smoothly with avplay. One line fix in compute_frame_duration.
>>
>>
>
> @@ -1589,7 +1589,7 @@ static int output_packet(InputStream *ist, int
> ist_index,
> }
> ist->next_pts = ist->pts =
> guess_correct_pts(&ist->pts_ctx, picture.pkt_pts, picture.pkt_dts);
> if (ist->st->codec->time_base.num != 0) {
> -int ticks= ist->st->parser ?
> ist->st->parser->repeat_pict+1 : ist->st->codec->ticks_per_frame;
> +int ticks= ist->st->parser ?
> ist->st->parser->repeat_pict+ist->st->codec->ticks_per_frame :
> + ist->st->codec->ticks_per_frame;
>
> int ticks = st->st->codec->ticks_per_frame + (ist->st->parser ?
> ist->st->parser->repeat_pict : 0);
>
Well, after all that, I did some more code reading and spelunking of mailing
list archives and discovered
the reason that the code is the way it is. The decoders define repeat_pict ==
0 to mean one frame time because
a decoder always returns frames. The parser defines repeat_pict == 0 to mean
one field time because
the parser can return after parsing only a field. So when the parser sees a
frame, it must add 1
to repeat pict so that the caller can distinguish field from frame. I think
this is rather obtuse, but
it is what it is. Is anybody in favor of adding a field to
AVCodecParserContext to explicitly flag field
vs. frame? This option was discussed and rejected in the original mailing list
discussion.
This was all discussed in the context of this thread back in feb 2009.
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/84007/focus=84344
On a related note, the timebase for h.264 isn't limited to framerate and
framerate/2.
When the vui fixed_frame_rate flag is not set (VFR content), timebase can be
anything that
evenly divides the framerate. Thus ticks_per_frame should be framerate /
timebase.
And timebase shouldn't really be multiplied by 2 unconditionally. FYI,
handbrake
generates such files by default.
signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] make parsers' repeat_pict consistent with decoders
John Stebbins wrote:
On 08/25/2011 07:45 PM, John Stebbins wrote:
As we discussed on IRC this morning, this makes the value of repeat_pict
returned
by the parsers the same as the value returned by the decoders.
The parser was returning a value for repeat_pict that was the
actual number of repeats + 1.
Passes fate with the exception of 1 pre-existing failure
(fate-h264-conformance-mr3_tandberg_b).
Heh, fate passed, but some samples I have that use pulldown didn't play
smoothly with avplay. One line fix in compute_frame_duration.
@@ -1589,7 +1589,7 @@ static int output_packet(InputStream *ist, int ist_index,
}
ist->next_pts = ist->pts =
guess_correct_pts(&ist->pts_ctx, picture.pkt_pts, picture.pkt_dts);
if (ist->st->codec->time_base.num != 0) {
-int ticks= ist->st->parser ? ist->st->parser->repeat_pict+1 :
ist->st->codec->ticks_per_frame;
+int ticks= ist->st->parser ?
ist->st->parser->repeat_pict+ist->st->codec->ticks_per_frame :
+ ist->st->codec->ticks_per_frame;
int ticks = st->st->codec->ticks_per_frame + (ist->st->parser ?
ist->st->parser->repeat_pict : 0);
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] make parsers' repeat_pict consistent with decoders
On 08/25/2011 07:45 PM, John Stebbins wrote:
> As we discussed on IRC this morning, this makes the value of repeat_pict
> returned
> by the parsers the same as the value returned by the decoders.
>
> The parser was returning a value for repeat_pict that was the
> actual number of repeats + 1.
>
> Passes fate with the exception of 1 pre-existing failure
> (fate-h264-conformance-mr3_tandberg_b).
Heh, fate passed, but some samples I have that use pulldown didn't play
smoothly with avplay. One line fix in compute_frame_duration.
From dd49f30e1fdf33c06f530da102a3e968c1a1bb7c Mon Sep 17 00:00:00 2001
From: John Stebbins
Date: Thu, 25 Aug 2011 22:26:14 -0700
Subject: [PATCH] make parsers' repeat_pict consistent with decoders
The parser was returning a value for repeat_pict that was the
actual number of repeats + 1.
---
avconv.c |4 ++--
libavcodec/h264_parser.c | 25 ++---
libavcodec/mpegvideo_parser.c |9 +
libavcodec/vc1_parser.c | 19 ---
libavformat/utils.c |4 ++--
5 files changed, 27 insertions(+), 34 deletions(-)
diff --git a/avconv.c b/avconv.c
index 57721f8..62951fa 100644
--- a/avconv.c
+++ b/avconv.c
@@ -1589,7 +1589,7 @@ static int output_packet(InputStream *ist, int ist_index,
}
ist->next_pts = ist->pts = guess_correct_pts(&ist->pts_ctx, picture.pkt_pts, picture.pkt_dts);
if (ist->st->codec->time_base.num != 0) {
-int ticks= ist->st->parser ? ist->st->parser->repeat_pict+1 : ist->st->codec->ticks_per_frame;
+int ticks= ist->st->parser ? ist->st->parser->repeat_pict+ist->st->codec->ticks_per_frame : ist->st->codec->ticks_per_frame;
ist->next_pts += ((int64_t)AV_TIME_BASE *
ist->st->codec->time_base.num * ticks) /
ist->st->codec->time_base.den;
@@ -1620,7 +1620,7 @@ static int output_packet(InputStream *ist, int ist_index,
break;
case AVMEDIA_TYPE_VIDEO:
if (ist->st->codec->time_base.num != 0) {
-int ticks= ist->st->parser ? ist->st->parser->repeat_pict+1 : ist->st->codec->ticks_per_frame;
+int ticks= ist->st->parser ? ist->st->parser->repeat_pict+ist->st->codec->ticks_per_frame : ist->st->codec->ticks_per_frame;
ist->next_pts += ((int64_t)AV_TIME_BASE *
ist->st->codec->time_base.num * ticks) /
ist->st->codec->time_base.den;
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 5610269..050d30f 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -119,6 +119,7 @@ static inline int parse_nal_units(AVCodecParserContext *s,
/* set some sane default values */
s->pict_type = AV_PICTURE_TYPE_I;
s->key_frame = 0;
+s->repeat_pict = 0;
h->s.avctx= avctx;
h->sei_recovery_frame_cnt = -1;
@@ -202,31 +203,25 @@ static inline int parse_nal_units(AVCodecParserContext *s,
if(h->sps.pic_struct_present_flag) {
switch (h->sei_pic_struct) {
-case SEI_PIC_STRUCT_TOP_FIELD:
-case SEI_PIC_STRUCT_BOTTOM_FIELD:
-s->repeat_pict = 0;
-break;
-case SEI_PIC_STRUCT_FRAME:
-case SEI_PIC_STRUCT_TOP_BOTTOM:
-case SEI_PIC_STRUCT_BOTTOM_TOP:
-s->repeat_pict = 1;
-break;
case SEI_PIC_STRUCT_TOP_BOTTOM_TOP:
case SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM:
-s->repeat_pict = 2;
+s->repeat_pict = 1;
break;
case SEI_PIC_STRUCT_FRAME_DOUBLING:
-s->repeat_pict = 3;
+s->repeat_pict = 2;
break;
case SEI_PIC_STRUCT_FRAME_TRIPLING:
-s->repeat_pict = 5;
+s->repeat_pict = 3;
break;
+case SEI_PIC_STRUCT_TOP_FIELD:
+case SEI_PIC_STRUCT_BOTTOM_FIELD:
+case SEI_PIC_STRUCT_FRAME:
+case SEI_PIC_STRUCT_TOP_BOTTOM:
+case SEI_PIC_STRUCT_BOTTOM_TOP:
default:
-s->repeat_pict = h->s.picture_structure == PICT_FRAME ? 1 : 0;
+s->repeat_pict = 0;
break;
}
-} else {
-s->repeat_pict = h->s.picture_structure == PICT_FRAME ? 1 : 0;
}
return 0; /* no need to evaluate the rest */
diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_pars
