Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
On 9/14/16, Michael Niedermayer wrote:
> On Wed, Sep 14, 2016 at 04:42:59PM +0200, Paul B Mahol wrote:
>> Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert.
>>
>> Signed-off-by: Paul B Mahol
>> ---
>> libavcodec/h264_parser.c | 22 ++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
>> index 615884f..cf6c3d1 100644
>> --- a/libavcodec/h264_parser.c
>> +++ b/libavcodec/h264_parser.c
>> @@ -60,6 +60,7 @@ typedef struct H264ParseContext {
>> uint8_t parse_history[6];
>> int parse_history_count;
>> int parse_last_mb;
>> +int64_t reference_dts;
>> } H264ParseContext;
>>
>>
>> @@ -598,6 +599,26 @@ static int h264_parse(AVCodecParserContext *s,
>> s->flags &= PARSER_FLAG_COMPLETE_FRAMES;
>> }
>>
>> +if (s->dts_sync_point >= 0) {
>> +int64_t den = avctx->time_base.den * avctx->pkt_timebase.num;
>> +if (den > 0) {
>> +int64_t num = avctx->time_base.num *
>> avctx->pkt_timebase.den;
>
> overflows, needs this:
> @@ -600,9 +600,9 @@ static int h264_parse(AVCodecParserContext *s,
> }
>
> if (s->dts_sync_point >= 0) {
> -int64_t den = avctx->time_base.den * avctx->pkt_timebase.num;
> +int64_t den = avctx->time_base.den *
> (int64_t)avctx->pkt_timebase.num;
> if (den > 0) {
> -int64_t num = avctx->time_base.num * avctx->pkt_timebase.den;
> +int64_t num = avctx->time_base.num *
> (int64_t)avctx->pkt_timebase.den;
> if (s->dts != AV_NOPTS_VALUE) {
> // got DTS from the stream, update reference timestamp
> p->reference_dts = s->dts - s->dts_ref_dts_delta * num /
> den;
>
>
> LGTM otherwise
>
> thanks
Sorry, I completely forgot to do this in same patch, got sidetracked
with av_rescale.
Applied.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
Hi! 2016-09-14 19:35 GMT+02:00 Paul B Mahol : > I see nothing wrong doing this in parser instead of in generic code. I am not saying there is anything wrong. I wanted to share my suspicion that this patch is a work-around for a bug that affects h.264 decoding in general. (And this suspicion may of course be wrong.) Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
On 9/14/16, Carl Eugen Hoyos wrote: > 2016-09-14 18:11 GMT+02:00 Paul B Mahol : >> On Wednesday, September 14, 2016, Carl Eugen Hoyos wrote: >> >>> 2016-09-14 16:42 GMT+02:00 Paul B Mahol : >>> > Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert. >>> >>> I am of course not against this patch but wasn't the reason for the >>> old code in utils.c that our h264 parser has insufficiencies? >> >> Like what? Only h264 have those fields. Doing it outside of >> parser seems strange. > > I thought - and please correct me if this is wrong - that this code was > always (only?) needed because our h264 parser does not do what > the specification requires to correctly determine h264 timestamps. This code appears to be needed for field based h264 streams. The code that was reverted was added in 2009. and than there was no avctx->pkt_timebase. I see nothing wrong doing this in parser instead of in generic code. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
2016-09-14 18:11 GMT+02:00 Paul B Mahol : > On Wednesday, September 14, 2016, Carl Eugen Hoyos wrote: > >> 2016-09-14 16:42 GMT+02:00 Paul B Mahol : >> > Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert. >> >> I am of course not against this patch but wasn't the reason for the >> old code in utils.c that our h264 parser has insufficiencies? > > Like what? Only h264 have those fields. Doing it outside of > parser seems strange. I thought - and please correct me if this is wrong - that this code was always (only?) needed because our h264 parser does not do what the specification requires to correctly determine h264 timestamps. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
On Wednesday, September 14, 2016, Carl Eugen Hoyos wrote: > 2016-09-14 16:42 GMT+02:00 Paul B Mahol >: > > Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert. > > I am of course not against this patch but wasn't the reason for the > old code in utils.c that our h264 parser has insufficiencies? > Like what? Only h264 have those fields. Doing it outside of parser seems strange. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
On Wed, Sep 14, 2016 at 04:42:59PM +0200, Paul B Mahol wrote:
> Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert.
>
> Signed-off-by: Paul B Mahol
> ---
> libavcodec/h264_parser.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
> index 615884f..cf6c3d1 100644
> --- a/libavcodec/h264_parser.c
> +++ b/libavcodec/h264_parser.c
> @@ -60,6 +60,7 @@ typedef struct H264ParseContext {
> uint8_t parse_history[6];
> int parse_history_count;
> int parse_last_mb;
> +int64_t reference_dts;
> } H264ParseContext;
>
>
> @@ -598,6 +599,26 @@ static int h264_parse(AVCodecParserContext *s,
> s->flags &= PARSER_FLAG_COMPLETE_FRAMES;
> }
>
> +if (s->dts_sync_point >= 0) {
> +int64_t den = avctx->time_base.den * avctx->pkt_timebase.num;
> +if (den > 0) {
> +int64_t num = avctx->time_base.num * avctx->pkt_timebase.den;
overflows, needs this:
@@ -600,9 +600,9 @@ static int h264_parse(AVCodecParserContext *s,
}
if (s->dts_sync_point >= 0) {
-int64_t den = avctx->time_base.den * avctx->pkt_timebase.num;
+int64_t den = avctx->time_base.den * (int64_t)avctx->pkt_timebase.num;
if (den > 0) {
-int64_t num = avctx->time_base.num * avctx->pkt_timebase.den;
+int64_t num = avctx->time_base.num *
(int64_t)avctx->pkt_timebase.den;
if (s->dts != AV_NOPTS_VALUE) {
// got DTS from the stream, update reference timestamp
p->reference_dts = s->dts - s->dts_ref_dts_delta * num / den;
LGTM otherwise
thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
2016-09-14 16:42 GMT+02:00 Paul B Mahol : > Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert. I am of course not against this patch but wasn't the reason for the old code in utils.c that our h264 parser has insufficiencies? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames
Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert.
Signed-off-by: Paul B Mahol
---
libavcodec/h264_parser.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 615884f..cf6c3d1 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -60,6 +60,7 @@ typedef struct H264ParseContext {
uint8_t parse_history[6];
int parse_history_count;
int parse_last_mb;
+int64_t reference_dts;
} H264ParseContext;
@@ -598,6 +599,26 @@ static int h264_parse(AVCodecParserContext *s,
s->flags &= PARSER_FLAG_COMPLETE_FRAMES;
}
+if (s->dts_sync_point >= 0) {
+int64_t den = avctx->time_base.den * avctx->pkt_timebase.num;
+if (den > 0) {
+int64_t num = avctx->time_base.num * avctx->pkt_timebase.den;
+if (s->dts != AV_NOPTS_VALUE) {
+// got DTS from the stream, update reference timestamp
+p->reference_dts = s->dts - s->dts_ref_dts_delta * num / den;
+} else if (p->reference_dts != AV_NOPTS_VALUE) {
+// compute DTS based on reference timestamp
+s->dts = p->reference_dts + s->dts_ref_dts_delta * num / den;
+}
+
+if (p->reference_dts != AV_NOPTS_VALUE && s->pts == AV_NOPTS_VALUE)
+s->pts = s->dts + s->pts_dts_delta * num / den;
+
+if (s->dts_sync_point > 0)
+p->reference_dts = s->dts; // new reference
+}
+}
+
*poutbuf = buf;
*poutbuf_size = buf_size;
return next;
@@ -655,6 +676,7 @@ static av_cold int init(AVCodecParserContext *s)
{
H264ParseContext *p = s->priv_data;
+p->reference_dts = AV_NOPTS_VALUE;
ff_h264dsp_init(&p->h264dsp, 8, 1);
return 0;
}
--
2.5.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
