Re: [FFmpeg-devel] [PATCH] avcodec/h264_parser: set missing pts for top/bottom field frames

[Paul B Mahol]

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

[Carl Eugen Hoyos]

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

[Paul B Mahol]

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

[Carl Eugen Hoyos]

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

[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.
___
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

[Michael Niedermayer]

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

[Carl Eugen Hoyos]

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

[Paul B Mahol]

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(>h264dsp, 8, 1);
 return 0;
 }
-- 
2.5.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel