Re: [FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Vittorio Giovara]

On Mon, Nov 7, 2016 at 6:44 PM, Michael Niedermayer
 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.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Michael Niedermayer]

On Mon, Nov 07, 2016 at 04:52:23PM -0500, Vittorio Giovara wrote:
> On Sat, Nov 5, 2016 at 9:21 AM, Michael Niedermayer
>  wrote:
> > On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote:
> >> Signed-off-by: Vittorio Giovara 
> >> ---
> >> 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,
> >> +_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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Vittorio Giovara]

On Sat, Nov 5, 2016 at 9:21 AM, Michael Niedermayer
 wrote:
> On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote:
>> Signed-off-by: Vittorio Giovara 
>> ---
>> 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,
>> +_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).
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Michael Niedermayer]

On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote:
> Signed-off-by: Vittorio Giovara 
> ---
> 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,
> +_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.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel