| Message ID | 20161102154858.28628-2-vittorio.giovara@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote: > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > --- > Please CC. > Vittorio > > libavcodec/hevc.c | 18 ++++++++++++++++++ > libavformat/mov.c | 4 ---- > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c > index 29e0d49..b50120e 100644 > --- a/libavcodec/hevc.c > +++ b/libavcodec/hevc.c > @@ -3051,6 +3051,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > AVPacket *avpkt) > { > int ret; > + int new_extradata_size; > + uint8_t *new_extradata; > HEVCContext *s = avctx->priv_data; > > if (!avpkt->size) { > @@ -3062,6 +3064,22 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > return 0; > } > > + new_extradata_size = 0; > + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > + &new_extradata_size); > + if (new_extradata_size > 0 && new_extradata) { new_extradata should be checked first, that should make new_extradata_size = 0; unneeded > + if (new_extradata_size > avctx->extradata_size) { > + avctx->extradata = av_realloc(avctx->extradata, new_extradata_size); This leaks on reallocation failure overwriting the allocated pointer also can you add a fate test ? thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them.
On Sat, Nov 5, 2016 at 9:21 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote: >> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >> --- >> Please CC. >> Vittorio >> >> libavcodec/hevc.c | 18 ++++++++++++++++++ >> libavformat/mov.c | 4 ---- >> 2 files changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c >> index 29e0d49..b50120e 100644 >> --- a/libavcodec/hevc.c >> +++ b/libavcodec/hevc.c >> @@ -3051,6 +3051,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, >> AVPacket *avpkt) >> { >> int ret; >> + int new_extradata_size; >> + uint8_t *new_extradata; >> HEVCContext *s = avctx->priv_data; >> >> if (!avpkt->size) { >> @@ -3062,6 +3064,22 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, >> return 0; >> } >> >> + new_extradata_size = 0; >> + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, >> + &new_extradata_size); >> + if (new_extradata_size > 0 && new_extradata) { > > new_extradata should be checked first, that should make > new_extradata_size = 0; unneeded ok >> + if (new_extradata_size > avctx->extradata_size) { > >> + avctx->extradata = av_realloc(avctx->extradata, new_extradata_size); > > This leaks on reallocation failure overwriting the allocated pointer yeah, also, extradata is av_malloc'd, it is not possible call any *realloc* functions at all. I thought of av_free + av_malloc but I'm afraid a free there will interfere with frame multi threading, like it did for h264. So maybe it's better to modify hevc_decode_extradata() to support reading extradata from input buffers rather than from avctx (like h264 does). Thoughts? > also can you add a fate test ? I have a sample I can share but it's incredibly big for a fate test (85mb).
On Mon, Nov 07, 2016 at 04:52:23PM -0500, Vittorio Giovara wrote: > On Sat, Nov 5, 2016 at 9:21 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote: > >> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > >> --- > >> Please CC. > >> Vittorio > >> > >> libavcodec/hevc.c | 18 ++++++++++++++++++ > >> libavformat/mov.c | 4 ---- > >> 2 files changed, 18 insertions(+), 4 deletions(-) > >> > >> diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c > >> index 29e0d49..b50120e 100644 > >> --- a/libavcodec/hevc.c > >> +++ b/libavcodec/hevc.c > >> @@ -3051,6 +3051,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > >> AVPacket *avpkt) > >> { > >> int ret; > >> + int new_extradata_size; > >> + uint8_t *new_extradata; > >> HEVCContext *s = avctx->priv_data; > >> > >> if (!avpkt->size) { > >> @@ -3062,6 +3064,22 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > >> return 0; > >> } > >> > >> + new_extradata_size = 0; > >> + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > >> + &new_extradata_size); > >> + if (new_extradata_size > 0 && new_extradata) { > > > > new_extradata should be checked first, that should make > > new_extradata_size = 0; unneeded > > ok > > >> + if (new_extradata_size > avctx->extradata_size) { > > > >> + avctx->extradata = av_realloc(avctx->extradata, new_extradata_size); > > > > This leaks on reallocation failure overwriting the allocated pointer > > yeah, also, extradata is av_malloc'd, it is not possible call any > *realloc* functions at all. > > I thought of av_free + av_malloc but I'm afraid a free there will > interfere with frame multi threading, like it did for h264. So maybe > it's better to modify hevc_decode_extradata() to support reading > extradata from input buffers rather than from avctx (like h264 does). > Thoughts? the decoder should not change extradata, yes directly reading would be best probably > > > also can you add a fate test ? > > I have a sample I can share but it's incredibly big for a fate test (85mb). :( cant it be cut and glued so its smaller ? thx [...]
On Mon, Nov 7, 2016 at 6:44 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > the decoder should not change extradata, yes directly reading would > be best probably ok I'll amend. >> >> > also can you add a fate test ? >> >> I have a sample I can share but it's incredibly big for a fate test (85mb). > > :( > > cant it be cut and glued so its smaller ? Someone tipped me of a way, I'll try.
diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c index 29e0d49..b50120e 100644 --- a/libavcodec/hevc.c +++ b/libavcodec/hevc.c @@ -3051,6 +3051,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, AVPacket *avpkt) { int ret; + int new_extradata_size; + uint8_t *new_extradata; HEVCContext *s = avctx->priv_data; if (!avpkt->size) { @@ -3062,6 +3064,22 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, return 0; } + new_extradata_size = 0; + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, + &new_extradata_size); + if (new_extradata_size > 0 && new_extradata) { + if (new_extradata_size > avctx->extradata_size) { + avctx->extradata = av_realloc(avctx->extradata, new_extradata_size); + if (!avctx->extradata) + return AVERROR(ENOMEM); + } + avctx->extradata_size = new_extradata_size; + memcpy(avctx->extradata, new_extradata, new_extradata_size); + ret = hevc_decode_extradata(s); + if (ret < 0) + return ret; + } + s->ref = NULL; ret = decode_nal_units(s, avpkt->data, avpkt->size); if (ret < 0) diff --git a/libavformat/mov.c b/libavformat/mov.c index 4222088..24c75ab 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2212,10 +2212,6 @@ static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb, avio_skip(pb, size); return 1; } - if ( codec_tag == AV_RL32("hvc1") || - codec_tag == AV_RL32("hev1") - ) - av_log(c->fc, AV_LOG_WARNING, "Concatenated H.264 or H.265 might not play correctly.\n"); return 0; }
Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> --- Please CC. Vittorio libavcodec/hevc.c | 18 ++++++++++++++++++ libavformat/mov.c | 4 ---- 2 files changed, 18 insertions(+), 4 deletions(-)