| Message ID | 1473864179-4509-1-git-send-email-onemda@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
2016-09-14 16:42 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
> 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
On Wed, Sep 14, 2016 at 04:42:59PM +0200, Paul B Mahol wrote: > Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > 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 [...]
On Wednesday, September 14, 2016, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-09-14 16:42 GMT+02:00 Paul B Mahol <onemda@gmail.com <javascript:;>>: > > 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.
2016-09-14 18:11 GMT+02:00 Paul B Mahol <onemda@gmail.com>: > On Wednesday, September 14, 2016, Carl Eugen Hoyos wrote: > >> 2016-09-14 16:42 GMT+02:00 Paul B Mahol <onemda@gmail.com>: >> > 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
On 9/14/16, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-09-14 18:11 GMT+02:00 Paul B Mahol <onemda@gmail.com>: >> On Wednesday, September 14, 2016, Carl Eugen Hoyos wrote: >> >>> 2016-09-14 16:42 GMT+02:00 Paul B Mahol <onemda@gmail.com>: >>> > 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.
Hi!
2016-09-14 19:35 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
> 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
On 9/14/16, Michael Niedermayer <michael@niedermayer.cc> 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 <onemda@gmail.com> >> --- >> 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.
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; }
Adopted from 4eb49fdde8f84d54a763cfb5d355527b525ee2bf revert. Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/h264_parser.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)