[FFmpeg-devel,1/2] avcodec/avpacket: Use av_copy_packet_side_data() in av_packet_copy_props()

Submitted by Michael Niedermayer on May 3, 2017, 3:21 a.m.

Details

Message ID 20170503032151.25225-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer May 3, 2017, 3:21 a.m.
Fixes timeout
Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/avpacket.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

wm4 May 3, 2017, 3:29 a.m.
On Wed,  3 May 2017 05:21:50 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Fixes timeout
> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avpacket.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 4bf830bb8a..ff7ee730a4 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      dst->flags                = src->flags;
>      dst->stream_index         = src->stream_index;
>  
> +    if (!dst->side_data_elems);
> +        return av_copy_packet_side_data(dst, src);
> +
>      for (i = 0; i < src->side_data_elems; i++) {
>           enum AVPacketSideDataType type = src->side_data[i].type;
>           int size          = src->side_data[i].size;

This doesn't look right...
James Almer May 3, 2017, 3:46 a.m.
On 5/3/2017 12:21 AM, Michael Niedermayer wrote:
> Fixes timeout
> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avpacket.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 4bf830bb8a..ff7ee730a4 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      dst->flags                = src->flags;
>      dst->stream_index         = src->stream_index;
>  
> +    if (!dst->side_data_elems);
> +        return av_copy_packet_side_data(dst, src);.

This is a deprecated function, so ideally you'd use something else.

How does this fixes the problem anyway? The code below copies the
packet's side data as well.

> +
>      for (i = 0; i < src->side_data_elems; i++) {
>           enum AVPacketSideDataType type = src->side_data[i].type;
>           int size          = src->side_data[i].size;
>
Michael Niedermayer May 3, 2017, 9:29 a.m.
On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> On Wed,  3 May 2017 05:21:50 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > Fixes timeout
> > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avpacket.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > index 4bf830bb8a..ff7ee730a4 100644
> > --- a/libavcodec/avpacket.c
> > +++ b/libavcodec/avpacket.c
> > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >      dst->flags                = src->flags;
> >      dst->stream_index         = src->stream_index;
> >  
> > +    if (!dst->side_data_elems);
> > +        return av_copy_packet_side_data(dst, src);
> > +
> >      for (i = 0; i < src->side_data_elems; i++) {
> >           enum AVPacketSideDataType type = src->side_data[i].type;
> >           int size          = src->side_data[i].size;
> 
> This doesn't look right...

already fixed the ; locally


[...]
wm4 May 3, 2017, 9:37 a.m.
On Wed, 3 May 2017 11:29:04 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> > On Wed,  3 May 2017 05:21:50 +0200
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > Fixes timeout
> > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/avpacket.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > index 4bf830bb8a..ff7ee730a4 100644
> > > --- a/libavcodec/avpacket.c
> > > +++ b/libavcodec/avpacket.c
> > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > >      dst->flags                = src->flags;
> > >      dst->stream_index         = src->stream_index;
> > >  
> > > +    if (!dst->side_data_elems);
> > > +        return av_copy_packet_side_data(dst, src);
> > > +
> > >      for (i = 0; i < src->side_data_elems; i++) {
> > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > >           int size          = src->side_data[i].size;  
> > 
> > This doesn't look right...  
> 
> already fixed the ; locally
> 
> 
> [...]

I didn't see that, I was referring to the fact that you call
av_copy_packet_side_data(), and only sometimes (at least by intention).
That requires at least an explanation in the commit message.
Michael Niedermayer May 3, 2017, 9:45 a.m.
On Wed, May 03, 2017 at 12:46:29AM -0300, James Almer wrote:
> On 5/3/2017 12:21 AM, Michael Niedermayer wrote:
> > Fixes timeout
> > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avpacket.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > index 4bf830bb8a..ff7ee730a4 100644
> > --- a/libavcodec/avpacket.c
> > +++ b/libavcodec/avpacket.c
> > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >      dst->flags                = src->flags;
> >      dst->stream_index         = src->stream_index;
> >  
> > +    if (!dst->side_data_elems);
> > +        return av_copy_packet_side_data(dst, src);.
> 
> This is a deprecated function, so ideally you'd use something else.

av_copy_packet_side_data() isnt marked as deprecated in avcodec.h


> 
> How does this fixes the problem anyway? The code below copies the
> packet's side data as well.

I suspect calling realloc() repetly to add 1 element each time is
expensive in some cases like at least some memory debuggers/checkers

It should always be better to avoid repeatly reallocating

[...]
Michael Niedermayer May 3, 2017, 9:50 a.m.
On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
> On Wed, 3 May 2017 11:29:04 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> > > On Wed,  3 May 2017 05:21:50 +0200
> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >   
> > > > Fixes timeout
> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/avpacket.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > > index 4bf830bb8a..ff7ee730a4 100644
> > > > --- a/libavcodec/avpacket.c
> > > > +++ b/libavcodec/avpacket.c
> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > >      dst->flags                = src->flags;
> > > >      dst->stream_index         = src->stream_index;
> > > >  
> > > > +    if (!dst->side_data_elems);
> > > > +        return av_copy_packet_side_data(dst, src);
> > > > +
> > > >      for (i = 0; i < src->side_data_elems; i++) {
> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > > >           int size          = src->side_data[i].size;  
> > > 
> > > This doesn't look right...  
> > 
> > already fixed the ; locally
> > 
> > 
> > [...]
> 
> I didn't see that, I was referring to the fact that you call
> av_copy_packet_side_data(), and only sometimes (at least by intention).
> That requires at least an explanation in the commit message.

av_packet_copy_props() would add side data to the destination packet
it doesnt replace previously existing side data except in case of
error.
I dont know if that is intended but i didnt want to change it as that
would be unrelated to this patch

[...]
Michael Niedermayer May 3, 2017, 9:55 a.m.
On Wed, May 03, 2017 at 11:50:41AM +0200, Michael Niedermayer wrote:
> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
> > On Wed, 3 May 2017 11:29:04 +0200
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > 
> > > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> > > > On Wed,  3 May 2017 05:21:50 +0200
> > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > >   
> > > > > Fixes timeout
> > > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > > > 
> > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/avpacket.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > > > index 4bf830bb8a..ff7ee730a4 100644
> > > > > --- a/libavcodec/avpacket.c
> > > > > +++ b/libavcodec/avpacket.c
> > > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > > >      dst->flags                = src->flags;
> > > > >      dst->stream_index         = src->stream_index;
> > > > >  
> > > > > +    if (!dst->side_data_elems);
> > > > > +        return av_copy_packet_side_data(dst, src);
> > > > > +
> > > > >      for (i = 0; i < src->side_data_elems; i++) {
> > > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > > > >           int size          = src->side_data[i].size;  
> > > > 
> > > > This doesn't look right...  
> > > 
> > > already fixed the ; locally
> > > 
> > > 
> > > [...]
> > 
> > I didn't see that, I was referring to the fact that you call
> > av_copy_packet_side_data(), and only sometimes (at least by intention).
> > That requires at least an explanation in the commit message.
> 
> av_packet_copy_props() would add side data to the destination packet
> it doesnt replace previously existing side data except in case of
> error.
> I dont know if that is intended but i didnt want to change it as that
> would be unrelated to this patch

added
"av_copy_packet_side_data() is only used when it does not lead to a change in behaviour"
to the commit message

[...]
wm4 May 3, 2017, 10:07 a.m.
On Wed, 3 May 2017 11:55:16 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, May 03, 2017 at 11:50:41AM +0200, Michael Niedermayer wrote:
> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
> > > On Wed, 3 May 2017 11:29:04 +0200
> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >   
> > > > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
> > > > > On Wed,  3 May 2017 05:21:50 +0200
> > > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > >     
> > > > > > Fixes timeout
> > > > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > > > > 
> > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > >  libavcodec/avpacket.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > > > > index 4bf830bb8a..ff7ee730a4 100644
> > > > > > --- a/libavcodec/avpacket.c
> > > > > > +++ b/libavcodec/avpacket.c
> > > > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > > > >      dst->flags                = src->flags;
> > > > > >      dst->stream_index         = src->stream_index;
> > > > > >  
> > > > > > +    if (!dst->side_data_elems);
> > > > > > +        return av_copy_packet_side_data(dst, src);
> > > > > > +
> > > > > >      for (i = 0; i < src->side_data_elems; i++) {
> > > > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > > > > >           int size          = src->side_data[i].size;    
> > > > > 
> > > > > This doesn't look right...    
> > > > 
> > > > already fixed the ; locally
> > > > 
> > > > 
> > > > [...]  
> > > 
> > > I didn't see that, I was referring to the fact that you call
> > > av_copy_packet_side_data(), and only sometimes (at least by intention).
> > > That requires at least an explanation in the commit message.  
> > 
> > av_packet_copy_props() would add side data to the destination packet
> > it doesnt replace previously existing side data except in case of
> > error.
> > I dont know if that is intended but i didnt want to change it as that
> > would be unrelated to this patch  
> 
> added
> "av_copy_packet_side_data() is only used when it does not lead to a change in behaviour"
> to the commit message

Sorry, that seems all kinds of hacky, especially with the "suspected"
reason that the function is unexpectedly faster than the existing code
with some memory analyzers. It just doesn't make sense to me.
Hendrik Leppkes May 3, 2017, 10:54 a.m.
On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
>> On Wed, 3 May 2017 11:29:04 +0200
>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>
>> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
>> > > On Wed,  3 May 2017 05:21:50 +0200
>> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > >
>> > > > Fixes timeout
>> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>> > > >
>> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > > > ---
>> > > >  libavcodec/avpacket.c | 3 +++
>> > > >  1 file changed, 3 insertions(+)
>> > > >
>> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> > > > index 4bf830bb8a..ff7ee730a4 100644
>> > > > --- a/libavcodec/avpacket.c
>> > > > +++ b/libavcodec/avpacket.c
>> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>> > > >      dst->flags                = src->flags;
>> > > >      dst->stream_index         = src->stream_index;
>> > > >
>> > > > +    if (!dst->side_data_elems);
>> > > > +        return av_copy_packet_side_data(dst, src);
>> > > > +
>> > > >      for (i = 0; i < src->side_data_elems; i++) {
>> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
>> > > >           int size          = src->side_data[i].size;
>> > >
>> > > This doesn't look right...
>> >
>> > already fixed the ; locally
>> >
>> >
>> > [...]
>>
>> I didn't see that, I was referring to the fact that you call
>> av_copy_packet_side_data(), and only sometimes (at least by intention).
>> That requires at least an explanation in the commit message.
>
> av_packet_copy_props() would add side data to the destination packet
> it doesnt replace previously existing side data except in case of
> error.
> I dont know if that is intended but i didnt want to change it as that
> would be unrelated to this patch
>

This behavior seems odd at best, so maybe we should just change that
and make the behavior more logical, and fix your issue at the same
time?

- Hendrik
Michael Niedermayer May 3, 2017, 1:16 p.m.
On Wed, May 03, 2017 at 12:07:46PM +0200, wm4 wrote:
> On Wed, 3 May 2017 11:55:16 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Wed, May 03, 2017 at 11:50:41AM +0200, Michael Niedermayer wrote:
> > > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
> > > > On Wed, 3 May 2017 11:29:04 +0200
> > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > >   
> > > > > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
> > > > > > On Wed,  3 May 2017 05:21:50 +0200
> > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > > >     
> > > > > > > Fixes timeout
> > > > > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > > > > > 
> > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > > ---
> > > > > > >  libavcodec/avpacket.c | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > > > > > index 4bf830bb8a..ff7ee730a4 100644
> > > > > > > --- a/libavcodec/avpacket.c
> > > > > > > +++ b/libavcodec/avpacket.c
> > > > > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > > > > >      dst->flags                = src->flags;
> > > > > > >      dst->stream_index         = src->stream_index;
> > > > > > >  
> > > > > > > +    if (!dst->side_data_elems);
> > > > > > > +        return av_copy_packet_side_data(dst, src);
> > > > > > > +
> > > > > > >      for (i = 0; i < src->side_data_elems; i++) {
> > > > > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > > > > > >           int size          = src->side_data[i].size;    
> > > > > > 
> > > > > > This doesn't look right...    
> > > > > 
> > > > > already fixed the ; locally
> > > > > 
> > > > > 
> > > > > [...]  
> > > > 
> > > > I didn't see that, I was referring to the fact that you call
> > > > av_copy_packet_side_data(), and only sometimes (at least by intention).
> > > > That requires at least an explanation in the commit message.  
> > > 
> > > av_packet_copy_props() would add side data to the destination packet
> > > it doesnt replace previously existing side data except in case of
> > > error.
> > > I dont know if that is intended but i didnt want to change it as that
> > > would be unrelated to this patch  
> > 
> > added
> > "av_copy_packet_side_data() is only used when it does not lead to a change in behaviour"
> > to the commit message
> 
> Sorry, that seems all kinds of hacky, especially with the "suspected"
> reason that the function is unexpectedly faster than the existing code
> with some memory analyzers. It just doesn't make sense to me.

Its not unexpected that O(N) is signifiantly slower than O(1)

The "suspect" comes only from myself not profiling the memory analyzer
to proof that the realloc() calls are what causes of the observed
significant speed difference


[...]
Michael Niedermayer May 3, 2017, 1:23 p.m.
On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
> >> On Wed, 3 May 2017 11:29:04 +0200
> >> Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>
> >> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> >> > > On Wed,  3 May 2017 05:21:50 +0200
> >> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > >
> >> > > > Fixes timeout
> >> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> >> > > >
> >> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> >> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > > > ---
> >> > > >  libavcodec/avpacket.c | 3 +++
> >> > > >  1 file changed, 3 insertions(+)
> >> > > >
> >> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >> > > > index 4bf830bb8a..ff7ee730a4 100644
> >> > > > --- a/libavcodec/avpacket.c
> >> > > > +++ b/libavcodec/avpacket.c
> >> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >> > > >      dst->flags                = src->flags;
> >> > > >      dst->stream_index         = src->stream_index;
> >> > > >
> >> > > > +    if (!dst->side_data_elems);
> >> > > > +        return av_copy_packet_side_data(dst, src);
> >> > > > +
> >> > > >      for (i = 0; i < src->side_data_elems; i++) {
> >> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> >> > > >           int size          = src->side_data[i].size;
> >> > >
> >> > > This doesn't look right...
> >> >
> >> > already fixed the ; locally
> >> >
> >> >
> >> > [...]
> >>
> >> I didn't see that, I was referring to the fact that you call
> >> av_copy_packet_side_data(), and only sometimes (at least by intention).
> >> That requires at least an explanation in the commit message.
> >
> > av_packet_copy_props() would add side data to the destination packet
> > it doesnt replace previously existing side data except in case of
> > error.
> > I dont know if that is intended but i didnt want to change it as that
> > would be unrelated to this patch
> >
> 
> This behavior seems odd at best, so maybe we should just change that
> and make the behavior more logical, and fix your issue at the same
> time?

That can be done and makes alot of sense after the patch.

we need to fix this issue in our releases too
a simple bugfix and a seperate behavior change afterwards allows us
to simply backport the bugfix from master.

That is unless theres consensus that the bevavior is a bug and should
be changed in affected releases?

[...]
Hendrik Leppkes May 3, 2017, 1:28 p.m.
On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
>> <michael@niedermayer.cc> wrote:
>> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
>> >> On Wed, 3 May 2017 11:29:04 +0200
>> >> Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >>
>> >> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
>> >> > > On Wed,  3 May 2017 05:21:50 +0200
>> >> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> > >
>> >> > > > Fixes timeout
>> >> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>> >> > > >
>> >> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>> >> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> >> > > > ---
>> >> > > >  libavcodec/avpacket.c | 3 +++
>> >> > > >  1 file changed, 3 insertions(+)
>> >> > > >
>> >> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> >> > > > index 4bf830bb8a..ff7ee730a4 100644
>> >> > > > --- a/libavcodec/avpacket.c
>> >> > > > +++ b/libavcodec/avpacket.c
>> >> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>> >> > > >      dst->flags                = src->flags;
>> >> > > >      dst->stream_index         = src->stream_index;
>> >> > > >
>> >> > > > +    if (!dst->side_data_elems);
>> >> > > > +        return av_copy_packet_side_data(dst, src);
>> >> > > > +
>> >> > > >      for (i = 0; i < src->side_data_elems; i++) {
>> >> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
>> >> > > >           int size          = src->side_data[i].size;
>> >> > >
>> >> > > This doesn't look right...
>> >> >
>> >> > already fixed the ; locally
>> >> >
>> >> >
>> >> > [...]
>> >>
>> >> I didn't see that, I was referring to the fact that you call
>> >> av_copy_packet_side_data(), and only sometimes (at least by intention).
>> >> That requires at least an explanation in the commit message.
>> >
>> > av_packet_copy_props() would add side data to the destination packet
>> > it doesnt replace previously existing side data except in case of
>> > error.
>> > I dont know if that is intended but i didnt want to change it as that
>> > would be unrelated to this patch
>> >
>>
>> This behavior seems odd at best, so maybe we should just change that
>> and make the behavior more logical, and fix your issue at the same
>> time?
>
> That can be done and makes alot of sense after the patch.
>
> we need to fix this issue in our releases too
> a simple bugfix and a seperate behavior change afterwards allows us
> to simply backport the bugfix from master.
>

If anything your "bugfix" here is a performance improvement, and I
don't think that warrants backporting either way.

In any case, it seems like this entire class of AVPacket functions
isn't mean to be called on pre-filled AVPackets, but instead only on
empty ones.
If you call av_packet_ref with a filled AVPacket as a dst, then it
looks like it would even leak its existing memory.

This does not seem to be documented properly, though, and overall
feels a bit silly to not just clear the dst packet before.

- Hendrik
wm4 May 3, 2017, 4:43 p.m.
On Wed, 3 May 2017 15:16:27 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, May 03, 2017 at 12:07:46PM +0200, wm4 wrote:
> > On Wed, 3 May 2017 11:55:16 +0200
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > On Wed, May 03, 2017 at 11:50:41AM +0200, Michael Niedermayer wrote:  
> > > > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:    
> > > > > On Wed, 3 May 2017 11:29:04 +0200
> > > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > >     
> > > > > > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:    
> > > > > > > On Wed,  3 May 2017 05:21:50 +0200
> > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > > > >       
> > > > > > > > Fixes timeout
> > > > > > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > > > > > > 
> > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > > > ---
> > > > > > > >  libavcodec/avpacket.c | 3 +++
> > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > > > > > > index 4bf830bb8a..ff7ee730a4 100644
> > > > > > > > --- a/libavcodec/avpacket.c
> > > > > > > > +++ b/libavcodec/avpacket.c
> > > > > > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > > > > > >      dst->flags                = src->flags;
> > > > > > > >      dst->stream_index         = src->stream_index;
> > > > > > > >  
> > > > > > > > +    if (!dst->side_data_elems);
> > > > > > > > +        return av_copy_packet_side_data(dst, src);
> > > > > > > > +
> > > > > > > >      for (i = 0; i < src->side_data_elems; i++) {
> > > > > > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > > > > > > >           int size          = src->side_data[i].size;      
> > > > > > > 
> > > > > > > This doesn't look right...      
> > > > > > 
> > > > > > already fixed the ; locally
> > > > > > 
> > > > > > 
> > > > > > [...]    
> > > > > 
> > > > > I didn't see that, I was referring to the fact that you call
> > > > > av_copy_packet_side_data(), and only sometimes (at least by intention).
> > > > > That requires at least an explanation in the commit message.    
> > > > 
> > > > av_packet_copy_props() would add side data to the destination packet
> > > > it doesnt replace previously existing side data except in case of
> > > > error.
> > > > I dont know if that is intended but i didnt want to change it as that
> > > > would be unrelated to this patch    
> > > 
> > > added
> > > "av_copy_packet_side_data() is only used when it does not lead to a change in behaviour"
> > > to the commit message  
> > 
> > Sorry, that seems all kinds of hacky, especially with the "suspected"
> > reason that the function is unexpectedly faster than the existing code
> > with some memory analyzers. It just doesn't make sense to me.  
> 
> Its not unexpected that O(N) is signifiantly slower than O(1)

It is when N is bounded by a constant (number of defined side data
types). And this is not only theoretically speaking. The constant is
small.

> The "suspect" comes only from myself not profiling the memory analyzer
> to proof that the realloc() calls are what causes of the observed
> significant speed difference
>
wm4 May 3, 2017, 4:46 p.m.
On Wed, 3 May 2017 15:28:53 +0200
Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:  
> >> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
> >> <michael@niedermayer.cc> wrote:  
> >> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
> >> >> On Wed, 3 May 2017 11:29:04 +0200
> >> >> Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >>  
> >> >> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
> >> >> > > On Wed,  3 May 2017 05:21:50 +0200
> >> >> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >> > >  
> >> >> > > > Fixes timeout
> >> >> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> >> >> > > >
> >> >> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> >> >> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> >> > > > ---
> >> >> > > >  libavcodec/avpacket.c | 3 +++
> >> >> > > >  1 file changed, 3 insertions(+)
> >> >> > > >
> >> >> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >> >> > > > index 4bf830bb8a..ff7ee730a4 100644
> >> >> > > > --- a/libavcodec/avpacket.c
> >> >> > > > +++ b/libavcodec/avpacket.c
> >> >> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >> >> > > >      dst->flags                = src->flags;
> >> >> > > >      dst->stream_index         = src->stream_index;
> >> >> > > >
> >> >> > > > +    if (!dst->side_data_elems);
> >> >> > > > +        return av_copy_packet_side_data(dst, src);
> >> >> > > > +
> >> >> > > >      for (i = 0; i < src->side_data_elems; i++) {
> >> >> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> >> >> > > >           int size          = src->side_data[i].size;  
> >> >> > >
> >> >> > > This doesn't look right...  
> >> >> >
> >> >> > already fixed the ; locally
> >> >> >
> >> >> >
> >> >> > [...]  
> >> >>
> >> >> I didn't see that, I was referring to the fact that you call
> >> >> av_copy_packet_side_data(), and only sometimes (at least by intention).
> >> >> That requires at least an explanation in the commit message.  
> >> >
> >> > av_packet_copy_props() would add side data to the destination packet
> >> > it doesnt replace previously existing side data except in case of
> >> > error.
> >> > I dont know if that is intended but i didnt want to change it as that
> >> > would be unrelated to this patch
> >> >  
> >>
> >> This behavior seems odd at best, so maybe we should just change that
> >> and make the behavior more logical, and fix your issue at the same
> >> time?  
> >
> > That can be done and makes alot of sense after the patch.
> >
> > we need to fix this issue in our releases too
> > a simple bugfix and a seperate behavior change afterwards allows us
> > to simply backport the bugfix from master.
> >  
> 
> If anything your "bugfix" here is a performance improvement, and I
> don't think that warrants backporting either way.

Apparently it only improves performance with seriously messed up fuzz
samples. Doesn't warrant more code complexity IMHO, especially not in
such a way.
Michael Niedermayer May 4, 2017, 2 a.m.
On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
> >> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
> >> <michael@niedermayer.cc> wrote:
> >> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
> >> >> On Wed, 3 May 2017 11:29:04 +0200
> >> >> Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >>
> >> >> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> >> >> > > On Wed,  3 May 2017 05:21:50 +0200
> >> >> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >> > >
> >> >> > > > Fixes timeout
> >> >> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> >> >> > > >
> >> >> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> >> >> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> >> > > > ---
> >> >> > > >  libavcodec/avpacket.c | 3 +++
> >> >> > > >  1 file changed, 3 insertions(+)
> >> >> > > >
> >> >> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >> >> > > > index 4bf830bb8a..ff7ee730a4 100644
> >> >> > > > --- a/libavcodec/avpacket.c
> >> >> > > > +++ b/libavcodec/avpacket.c
> >> >> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >> >> > > >      dst->flags                = src->flags;
> >> >> > > >      dst->stream_index         = src->stream_index;
> >> >> > > >
> >> >> > > > +    if (!dst->side_data_elems);
> >> >> > > > +        return av_copy_packet_side_data(dst, src);
> >> >> > > > +
> >> >> > > >      for (i = 0; i < src->side_data_elems; i++) {
> >> >> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> >> >> > > >           int size          = src->side_data[i].size;
> >> >> > >
> >> >> > > This doesn't look right...
> >> >> >
> >> >> > already fixed the ; locally
> >> >> >
> >> >> >
> >> >> > [...]
> >> >>
> >> >> I didn't see that, I was referring to the fact that you call
> >> >> av_copy_packet_side_data(), and only sometimes (at least by intention).
> >> >> That requires at least an explanation in the commit message.
> >> >
> >> > av_packet_copy_props() would add side data to the destination packet
> >> > it doesnt replace previously existing side data except in case of
> >> > error.
> >> > I dont know if that is intended but i didnt want to change it as that
> >> > would be unrelated to this patch
> >> >
> >>
> >> This behavior seems odd at best, so maybe we should just change that
> >> and make the behavior more logical, and fix your issue at the same
> >> time?
> >
> > That can be done and makes alot of sense after the patch.
> >
> > we need to fix this issue in our releases too
> > a simple bugfix and a seperate behavior change afterwards allows us
> > to simply backport the bugfix from master.
> >
> 
> If anything your "bugfix" here is a performance improvement, and I
> don't think that warrants backporting either way.

Before the patch the sample file takes
51seconds to decode, after it, it takes 203 milliseconds.
The difference is caused by only 2 calls to the copy code

blocking a machine for 51 seconds for something that decodes in 203ms
otherwise, is IMO worth backporting a fix for.

We can add a bound to the number of side data elements if theres
consens about doing that and doing it in releases.
still, no code should call realloc() in a loop when it can do one
(re)allocation call at the top.


[...]
James Almer May 4, 2017, 2:27 a.m.
On 5/3/2017 11:00 PM, Michael Niedermayer wrote:
> On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
>> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
>> <michael@niedermayer.cc> wrote:
>>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
>>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
>>>> <michael@niedermayer.cc> wrote:
>>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
>>>>>> On Wed, 3 May 2017 11:29:04 +0200
>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>
>>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
>>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>
>>>>>>>>> Fixes timeout
>>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>>>>>>>>>
>>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>> ---
>>>>>>>>>  libavcodec/avpacket.c | 3 +++
>>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>>>>>>      dst->flags                = src->flags;
>>>>>>>>>      dst->stream_index         = src->stream_index;
>>>>>>>>>
>>>>>>>>> +    if (!dst->side_data_elems);
>>>>>>>>> +        return av_copy_packet_side_data(dst, src);
>>>>>>>>> +
>>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
>>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
>>>>>>>>>           int size          = src->side_data[i].size;
>>>>>>>>
>>>>>>>> This doesn't look right...
>>>>>>>
>>>>>>> already fixed the ; locally
>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>
>>>>>> I didn't see that, I was referring to the fact that you call
>>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
>>>>>> That requires at least an explanation in the commit message.
>>>>>
>>>>> av_packet_copy_props() would add side data to the destination packet
>>>>> it doesnt replace previously existing side data except in case of
>>>>> error.
>>>>> I dont know if that is intended but i didnt want to change it as that
>>>>> would be unrelated to this patch
>>>>>
>>>>
>>>> This behavior seems odd at best, so maybe we should just change that
>>>> and make the behavior more logical, and fix your issue at the same
>>>> time?
>>>
>>> That can be done and makes alot of sense after the patch.
>>>
>>> we need to fix this issue in our releases too
>>> a simple bugfix and a seperate behavior change afterwards allows us
>>> to simply backport the bugfix from master.
>>>
>>
>> If anything your "bugfix" here is a performance improvement, and I
>> don't think that warrants backporting either way.
> 
> Before the patch the sample file takes
> 51seconds to decode, after it, it takes 203 milliseconds.
> The difference is caused by only 2 calls to the copy code
> 
> blocking a machine for 51 seconds for something that decodes in 203ms
> otherwise, is IMO worth backporting a fix for.
> 
> We can add a bound to the number of side data elements if theres
> consens about doing that and doing it in releases.
> still, no code should call realloc() in a loop when it can do one
> (re)allocation call at the top.

First we need to figure out if these side data copy functions are meant
to append the source packet's side data to the dest's, or if they should
replace it. That is, we need to see if these functions are meant to
assume the dest packet is empty or not. Because judging by every other
field copied, i guess it's implied the dest packet is expected to be empty.
It's worth nothing that av_stream_add_side_data() overwrites existing
side data if one of the same type already exists, unlike
av_packet_add_side_data().

After that we can adapt the code to do a single realloc, since depending
on the above decision the code will be different.
wm4 May 4, 2017, 2:45 a.m.
On Thu, 4 May 2017 04:00:39 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
> > On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
> > <michael@niedermayer.cc> wrote:  
> > > On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:  
> > >> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
> > >> <michael@niedermayer.cc> wrote:  
> > >> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
> > >> >> On Wed, 3 May 2017 11:29:04 +0200
> > >> >> Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >> >>  
> > >> >> > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
> > >> >> > > On Wed,  3 May 2017 05:21:50 +0200
> > >> >> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >> >> > >  
> > >> >> > > > Fixes timeout
> > >> >> > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > >> >> > > >
> > >> >> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > >> >> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >> >> > > > ---
> > >> >> > > >  libavcodec/avpacket.c | 3 +++
> > >> >> > > >  1 file changed, 3 insertions(+)
> > >> >> > > >
> > >> >> > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > >> >> > > > index 4bf830bb8a..ff7ee730a4 100644
> > >> >> > > > --- a/libavcodec/avpacket.c
> > >> >> > > > +++ b/libavcodec/avpacket.c
> > >> >> > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > >> >> > > >      dst->flags                = src->flags;
> > >> >> > > >      dst->stream_index         = src->stream_index;
> > >> >> > > >
> > >> >> > > > +    if (!dst->side_data_elems);
> > >> >> > > > +        return av_copy_packet_side_data(dst, src);
> > >> >> > > > +
> > >> >> > > >      for (i = 0; i < src->side_data_elems; i++) {
> > >> >> > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > >> >> > > >           int size          = src->side_data[i].size;  
> > >> >> > >
> > >> >> > > This doesn't look right...  
> > >> >> >
> > >> >> > already fixed the ; locally
> > >> >> >
> > >> >> >
> > >> >> > [...]  
> > >> >>
> > >> >> I didn't see that, I was referring to the fact that you call
> > >> >> av_copy_packet_side_data(), and only sometimes (at least by intention).
> > >> >> That requires at least an explanation in the commit message.  
> > >> >
> > >> > av_packet_copy_props() would add side data to the destination packet
> > >> > it doesnt replace previously existing side data except in case of
> > >> > error.
> > >> > I dont know if that is intended but i didnt want to change it as that
> > >> > would be unrelated to this patch
> > >> >  
> > >>
> > >> This behavior seems odd at best, so maybe we should just change that
> > >> and make the behavior more logical, and fix your issue at the same
> > >> time?  
> > >
> > > That can be done and makes alot of sense after the patch.
> > >
> > > we need to fix this issue in our releases too
> > > a simple bugfix and a seperate behavior change afterwards allows us
> > > to simply backport the bugfix from master.
> > >  
> > 
> > If anything your "bugfix" here is a performance improvement, and I
> > don't think that warrants backporting either way.  
> 
> Before the patch the sample file takes
> 51seconds to decode, after it, it takes 203 milliseconds.
> The difference is caused by only 2 calls to the copy code

All for a situation that shouldn't happen in a sane way anyway, not
even with heavily fuzzed files.

We should limit side data to 1 per type, or have only 1 piece of code
that copies side data (after careful consideration of potentially
changing semantics). Both would fix it, as far as I understand it.

> blocking a machine for 51 seconds for something that decodes in 203ms
> otherwise, is IMO worth backporting a fix for.

Is that needed? Isn't it a fuzzed sample, instead of some real world
case?
James Almer May 4, 2017, 3:09 a.m.
On 5/3/2017 6:45 AM, Michael Niedermayer wrote:
> On Wed, May 03, 2017 at 12:46:29AM -0300, James Almer wrote:
>> On 5/3/2017 12:21 AM, Michael Niedermayer wrote:
>>> Fixes timeout
>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/avpacket.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>> index 4bf830bb8a..ff7ee730a4 100644
>>> --- a/libavcodec/avpacket.c
>>> +++ b/libavcodec/avpacket.c
>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>      dst->flags                = src->flags;
>>>      dst->stream_index         = src->stream_index;
>>>  
>>> +    if (!dst->side_data_elems);
>>> +        return av_copy_packet_side_data(dst, src);.
>>
>> This is a deprecated function, so ideally you'd use something else.
> 
> av_copy_packet_side_data() isnt marked as deprecated in avcodec.h

It should be moved outside the relevant deprecation guards, then, or be
actually deprecated seeing it duplicates the functionality of the more
complete av_packet_copy_props (after its side data copying code is
optimized).

> 
> 
>>
>> How does this fixes the problem anyway? The code below copies the
>> packet's side data as well.
> 
> I suspect calling realloc() repetly to add 1 element each time is
> expensive in some cases like at least some memory debuggers/checkers
> 
> It should always be better to avoid repeatly reallocating
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer May 4, 2017, 10:51 a.m.
On Wed, May 03, 2017 at 11:27:53PM -0300, James Almer wrote:
> On 5/3/2017 11:00 PM, Michael Niedermayer wrote:
> > On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
> >> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
> >> <michael@niedermayer.cc> wrote:
> >>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
> >>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
> >>>> <michael@niedermayer.cc> wrote:
> >>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
> >>>>>> On Wed, 3 May 2017 11:29:04 +0200
> >>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>>>>>
> >>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
> >>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
> >>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>>>>>>>
> >>>>>>>>> Fixes timeout
> >>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> >>>>>>>>>
> >>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> >>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>>>>>> ---
> >>>>>>>>>  libavcodec/avpacket.c | 3 +++
> >>>>>>>>>  1 file changed, 3 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
> >>>>>>>>> --- a/libavcodec/avpacket.c
> >>>>>>>>> +++ b/libavcodec/avpacket.c
> >>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >>>>>>>>>      dst->flags                = src->flags;
> >>>>>>>>>      dst->stream_index         = src->stream_index;
> >>>>>>>>>
> >>>>>>>>> +    if (!dst->side_data_elems);
> >>>>>>>>> +        return av_copy_packet_side_data(dst, src);
> >>>>>>>>> +
> >>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
> >>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
> >>>>>>>>>           int size          = src->side_data[i].size;
> >>>>>>>>
> >>>>>>>> This doesn't look right...
> >>>>>>>
> >>>>>>> already fixed the ; locally
> >>>>>>>
> >>>>>>>
> >>>>>>> [...]
> >>>>>>
> >>>>>> I didn't see that, I was referring to the fact that you call
> >>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
> >>>>>> That requires at least an explanation in the commit message.
> >>>>>
> >>>>> av_packet_copy_props() would add side data to the destination packet
> >>>>> it doesnt replace previously existing side data except in case of
> >>>>> error.
> >>>>> I dont know if that is intended but i didnt want to change it as that
> >>>>> would be unrelated to this patch
> >>>>>
> >>>>
> >>>> This behavior seems odd at best, so maybe we should just change that
> >>>> and make the behavior more logical, and fix your issue at the same
> >>>> time?
> >>>
> >>> That can be done and makes alot of sense after the patch.
> >>>
> >>> we need to fix this issue in our releases too
> >>> a simple bugfix and a seperate behavior change afterwards allows us
> >>> to simply backport the bugfix from master.
> >>>
> >>
> >> If anything your "bugfix" here is a performance improvement, and I
> >> don't think that warrants backporting either way.
> > 
> > Before the patch the sample file takes
> > 51seconds to decode, after it, it takes 203 milliseconds.
> > The difference is caused by only 2 calls to the copy code
> > 
> > blocking a machine for 51 seconds for something that decodes in 203ms
> > otherwise, is IMO worth backporting a fix for.
> > 
> > We can add a bound to the number of side data elements if theres
> > consens about doing that and doing it in releases.
> > still, no code should call realloc() in a loop when it can do one
> > (re)allocation call at the top.
> 
> First we need to figure out if these side data copy functions are meant
> to append the source packet's side data to the dest's, or if they should
> replace it. That is, we need to see if these functions are meant to
> assume the dest packet is empty or not. Because judging by every other
> field copied, i guess it's implied the dest packet is expected to be empty.
> It's worth nothing that av_stream_add_side_data() overwrites existing
> side data if one of the same type already exists, unlike
> av_packet_add_side_data().

There are 2 Issues

A. Is that there is a bug in master and the releases that allows one
   to craft a input which will get you stuck in a realloc loop a long
   time
B. The API is not well documented about what should happen if the
   destination packet isnt empty on a side data copy

If we fix A and then fix B, then we can backport A without B.

If we fix B and then fix A then A will likely depend on B and backporting
just the fix without the API change could be hard.
Is there a consensus about backporting a change to this API documentation
and implementation aka "B" ?

A can be fixed by the proposed patch, by limiting the side data count
or by first defining the API more clearly aka B and then fixing it
along the lines of the clear definition.

What path do people prefer ?

[...]
Michael Niedermayer May 4, 2017, 1:38 p.m.
On Wed, May 03, 2017 at 05:21:50AM +0200, Michael Niedermayer wrote:
> Fixes timeout
> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496

This patch also fixes
1309/clusterfuzz-testcase-minimized-5754803370065920

[...]
James Almer May 4, 2017, 2:59 p.m.
On 5/4/2017 7:51 AM, Michael Niedermayer wrote:
> On Wed, May 03, 2017 at 11:27:53PM -0300, James Almer wrote:
>> On 5/3/2017 11:00 PM, Michael Niedermayer wrote:
>>> On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
>>>> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
>>>> <michael@niedermayer.cc> wrote:
>>>>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
>>>>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
>>>>>> <michael@niedermayer.cc> wrote:
>>>>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
>>>>>>>> On Wed, 3 May 2017 11:29:04 +0200
>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>
>>>>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
>>>>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
>>>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>>>
>>>>>>>>>>> Fixes timeout
>>>>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>>>>>>>>>>>
>>>>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>>>> ---
>>>>>>>>>>>  libavcodec/avpacket.c | 3 +++
>>>>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
>>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>>>>>>>>      dst->flags                = src->flags;
>>>>>>>>>>>      dst->stream_index         = src->stream_index;
>>>>>>>>>>>
>>>>>>>>>>> +    if (!dst->side_data_elems);
>>>>>>>>>>> +        return av_copy_packet_side_data(dst, src);
>>>>>>>>>>> +
>>>>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
>>>>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
>>>>>>>>>>>           int size          = src->side_data[i].size;
>>>>>>>>>>
>>>>>>>>>> This doesn't look right...
>>>>>>>>>
>>>>>>>>> already fixed the ; locally
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>
>>>>>>>> I didn't see that, I was referring to the fact that you call
>>>>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
>>>>>>>> That requires at least an explanation in the commit message.
>>>>>>>
>>>>>>> av_packet_copy_props() would add side data to the destination packet
>>>>>>> it doesnt replace previously existing side data except in case of
>>>>>>> error.
>>>>>>> I dont know if that is intended but i didnt want to change it as that
>>>>>>> would be unrelated to this patch
>>>>>>>
>>>>>>
>>>>>> This behavior seems odd at best, so maybe we should just change that
>>>>>> and make the behavior more logical, and fix your issue at the same
>>>>>> time?
>>>>>
>>>>> That can be done and makes alot of sense after the patch.
>>>>>
>>>>> we need to fix this issue in our releases too
>>>>> a simple bugfix and a seperate behavior change afterwards allows us
>>>>> to simply backport the bugfix from master.
>>>>>
>>>>
>>>> If anything your "bugfix" here is a performance improvement, and I
>>>> don't think that warrants backporting either way.
>>>
>>> Before the patch the sample file takes
>>> 51seconds to decode, after it, it takes 203 milliseconds.
>>> The difference is caused by only 2 calls to the copy code
>>>
>>> blocking a machine for 51 seconds for something that decodes in 203ms
>>> otherwise, is IMO worth backporting a fix for.
>>>
>>> We can add a bound to the number of side data elements if theres
>>> consens about doing that and doing it in releases.
>>> still, no code should call realloc() in a loop when it can do one
>>> (re)allocation call at the top.
>>
>> First we need to figure out if these side data copy functions are meant
>> to append the source packet's side data to the dest's, or if they should
>> replace it. That is, we need to see if these functions are meant to
>> assume the dest packet is empty or not. Because judging by every other
>> field copied, i guess it's implied the dest packet is expected to be empty.
>> It's worth nothing that av_stream_add_side_data() overwrites existing
>> side data if one of the same type already exists, unlike
>> av_packet_add_side_data().
> 
> There are 2 Issues
> 
> A. Is that there is a bug in master and the releases that allows one
>    to craft a input which will get you stuck in a realloc loop a long
>    time
> B. The API is not well documented about what should happen if the
>    destination packet isnt empty on a side data copy
> 
> If we fix A and then fix B, then we can backport A without B.
> 
> If we fix B and then fix A then A will likely depend on B and backporting
> just the fix without the API change could be hard.
> Is there a consensus about backporting a change to this API documentation
> and implementation aka "B" ?
> 
> A can be fixed by the proposed patch, by limiting the side data count
> or by first defining the API more clearly aka B and then fixing it
> along the lines of the clear definition.
> 
> What path do people prefer ?

Unless i'm reading this wrong, av_copy_packet_side_data() can leak on
allocation failure. It does dst->side_data_elems = src->side_data_elems
after everything was allocated and copied instead of doing
dst->side_data_elems++ per side data element successfully allocated and
copied.
Not counting that, your patch doesn't seem to change the current
av_packet_copy_props() behavior, which is important for backporting
purposes, so i guess it should be fine to apply.

After that, we'll need to properly document the API as requiring dst to
be empty (or stating it will be overwritten if populated) and adapt the
functions to do that. It's pretty evident it was never meant to be used
with populated packets.
wm4 May 4, 2017, 9:59 p.m.
On Thu, 4 May 2017 12:51:14 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, May 03, 2017 at 11:27:53PM -0300, James Almer wrote:
> > On 5/3/2017 11:00 PM, Michael Niedermayer wrote:  
> > > On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:  
> > >> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
> > >> <michael@niedermayer.cc> wrote:  
> > >>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:  
> > >>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
> > >>>> <michael@niedermayer.cc> wrote:  
> > >>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
> > >>>>>> On Wed, 3 May 2017 11:29:04 +0200
> > >>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >>>>>>  
> > >>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
> > >>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
> > >>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >>>>>>>>  
> > >>>>>>>>> Fixes timeout
> > >>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > >>>>>>>>>
> > >>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > >>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >>>>>>>>> ---
> > >>>>>>>>>  libavcodec/avpacket.c | 3 +++
> > >>>>>>>>>  1 file changed, 3 insertions(+)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > >>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
> > >>>>>>>>> --- a/libavcodec/avpacket.c
> > >>>>>>>>> +++ b/libavcodec/avpacket.c
> > >>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > >>>>>>>>>      dst->flags                = src->flags;
> > >>>>>>>>>      dst->stream_index         = src->stream_index;
> > >>>>>>>>>
> > >>>>>>>>> +    if (!dst->side_data_elems);
> > >>>>>>>>> +        return av_copy_packet_side_data(dst, src);
> > >>>>>>>>> +
> > >>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
> > >>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
> > >>>>>>>>>           int size          = src->side_data[i].size;  
> > >>>>>>>>
> > >>>>>>>> This doesn't look right...  
> > >>>>>>>
> > >>>>>>> already fixed the ; locally
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> [...]  
> > >>>>>>
> > >>>>>> I didn't see that, I was referring to the fact that you call
> > >>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
> > >>>>>> That requires at least an explanation in the commit message.  
> > >>>>>
> > >>>>> av_packet_copy_props() would add side data to the destination packet
> > >>>>> it doesnt replace previously existing side data except in case of
> > >>>>> error.
> > >>>>> I dont know if that is intended but i didnt want to change it as that
> > >>>>> would be unrelated to this patch
> > >>>>>  
> > >>>>
> > >>>> This behavior seems odd at best, so maybe we should just change that
> > >>>> and make the behavior more logical, and fix your issue at the same
> > >>>> time?  
> > >>>
> > >>> That can be done and makes alot of sense after the patch.
> > >>>
> > >>> we need to fix this issue in our releases too
> > >>> a simple bugfix and a seperate behavior change afterwards allows us
> > >>> to simply backport the bugfix from master.
> > >>>  
> > >>
> > >> If anything your "bugfix" here is a performance improvement, and I
> > >> don't think that warrants backporting either way.  
> > > 
> > > Before the patch the sample file takes
> > > 51seconds to decode, after it, it takes 203 milliseconds.
> > > The difference is caused by only 2 calls to the copy code
> > > 
> > > blocking a machine for 51 seconds for something that decodes in 203ms
> > > otherwise, is IMO worth backporting a fix for.
> > > 
> > > We can add a bound to the number of side data elements if theres
> > > consens about doing that and doing it in releases.
> > > still, no code should call realloc() in a loop when it can do one
> > > (re)allocation call at the top.  
> > 
> > First we need to figure out if these side data copy functions are meant
> > to append the source packet's side data to the dest's, or if they should
> > replace it. That is, we need to see if these functions are meant to
> > assume the dest packet is empty or not. Because judging by every other
> > field copied, i guess it's implied the dest packet is expected to be empty.
> > It's worth nothing that av_stream_add_side_data() overwrites existing
> > side data if one of the same type already exists, unlike
> > av_packet_add_side_data().  
> 
> There are 2 Issues
> 
> A. Is that there is a bug in master and the releases that allows one
>    to craft a input which will get you stuck in a realloc loop a long
>    time
> B. The API is not well documented about what should happen if the
>    destination packet isnt empty on a side data copy
> 
> If we fix A and then fix B, then we can backport A without B.
> 
> If we fix B and then fix A then A will likely depend on B and backporting
> just the fix without the API change could be hard.
> Is there a consensus about backporting a change to this API documentation
> and implementation aka "B" ?
> 
> A can be fixed by the proposed patch, by limiting the side data count
> or by first defining the API more clearly aka B and then fixing it
> along the lines of the clear definition.
> 
> What path do people prefer ?

If you feel strongly about it (and that it affects users), feel free to
push this patch to the release branches, but I think it should be
solved in master by making the code _less_ tricky, instead of _more_
tricky.

Regarding av_packet_copy_props(), I'd tend towards thinking that it
should merge the side data with the dst packet, which means if side
data of the same type is present in both packet, the side data of this
type is essentially removed from the dst packet and replaced with the
one from the source packet. Could probably be done by doing this inside
of av_packet_new_side_data().

(Unfortunate that all the side data code is idiotically duplicated in
other areas of the libs which use the side data concept.)
James Almer May 5, 2017, 1:55 a.m.
On 5/4/2017 6:59 PM, wm4 wrote:
> On Thu, 4 May 2017 12:51:14 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
>> On Wed, May 03, 2017 at 11:27:53PM -0300, James Almer wrote:
>>> On 5/3/2017 11:00 PM, Michael Niedermayer wrote:  
>>>> On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:  
>>>>> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
>>>>> <michael@niedermayer.cc> wrote:  
>>>>>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:  
>>>>>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
>>>>>>> <michael@niedermayer.cc> wrote:  
>>>>>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
>>>>>>>>> On Wed, 3 May 2017 11:29:04 +0200
>>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>>  
>>>>>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
>>>>>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
>>>>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>>>>  
>>>>>>>>>>>> Fixes timeout
>>>>>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>>>>>>>>>>>>
>>>>>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>>>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  libavcodec/avpacket.c | 3 +++
>>>>>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
>>>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>>>>>>>>>      dst->flags                = src->flags;
>>>>>>>>>>>>      dst->stream_index         = src->stream_index;
>>>>>>>>>>>>
>>>>>>>>>>>> +    if (!dst->side_data_elems);
>>>>>>>>>>>> +        return av_copy_packet_side_data(dst, src);
>>>>>>>>>>>> +
>>>>>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
>>>>>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
>>>>>>>>>>>>           int size          = src->side_data[i].size;  
>>>>>>>>>>>
>>>>>>>>>>> This doesn't look right...  
>>>>>>>>>>
>>>>>>>>>> already fixed the ; locally
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> [...]  
>>>>>>>>>
>>>>>>>>> I didn't see that, I was referring to the fact that you call
>>>>>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
>>>>>>>>> That requires at least an explanation in the commit message.  
>>>>>>>>
>>>>>>>> av_packet_copy_props() would add side data to the destination packet
>>>>>>>> it doesnt replace previously existing side data except in case of
>>>>>>>> error.
>>>>>>>> I dont know if that is intended but i didnt want to change it as that
>>>>>>>> would be unrelated to this patch
>>>>>>>>  
>>>>>>>
>>>>>>> This behavior seems odd at best, so maybe we should just change that
>>>>>>> and make the behavior more logical, and fix your issue at the same
>>>>>>> time?  
>>>>>>
>>>>>> That can be done and makes alot of sense after the patch.
>>>>>>
>>>>>> we need to fix this issue in our releases too
>>>>>> a simple bugfix and a seperate behavior change afterwards allows us
>>>>>> to simply backport the bugfix from master.
>>>>>>  
>>>>>
>>>>> If anything your "bugfix" here is a performance improvement, and I
>>>>> don't think that warrants backporting either way.  
>>>>
>>>> Before the patch the sample file takes
>>>> 51seconds to decode, after it, it takes 203 milliseconds.
>>>> The difference is caused by only 2 calls to the copy code
>>>>
>>>> blocking a machine for 51 seconds for something that decodes in 203ms
>>>> otherwise, is IMO worth backporting a fix for.
>>>>
>>>> We can add a bound to the number of side data elements if theres
>>>> consens about doing that and doing it in releases.
>>>> still, no code should call realloc() in a loop when it can do one
>>>> (re)allocation call at the top.  
>>>
>>> First we need to figure out if these side data copy functions are meant
>>> to append the source packet's side data to the dest's, or if they should
>>> replace it. That is, we need to see if these functions are meant to
>>> assume the dest packet is empty or not. Because judging by every other
>>> field copied, i guess it's implied the dest packet is expected to be empty.
>>> It's worth nothing that av_stream_add_side_data() overwrites existing
>>> side data if one of the same type already exists, unlike
>>> av_packet_add_side_data().  
>>
>> There are 2 Issues
>>
>> A. Is that there is a bug in master and the releases that allows one
>>    to craft a input which will get you stuck in a realloc loop a long
>>    time
>> B. The API is not well documented about what should happen if the
>>    destination packet isnt empty on a side data copy
>>
>> If we fix A and then fix B, then we can backport A without B.
>>
>> If we fix B and then fix A then A will likely depend on B and backporting
>> just the fix without the API change could be hard.
>> Is there a consensus about backporting a change to this API documentation
>> and implementation aka "B" ?
>>
>> A can be fixed by the proposed patch, by limiting the side data count
>> or by first defining the API more clearly aka B and then fixing it
>> along the lines of the clear definition.
>>
>> What path do people prefer ?
> 
> If you feel strongly about it (and that it affects users), feel free to
> push this patch to the release branches, but I think it should be
> solved in master by making the code _less_ tricky, instead of _more_
> tricky.
> 
> Regarding av_packet_copy_props(), I'd tend towards thinking that it
> should merge the side data with the dst packet, which means if side
> data of the same type is present in both packet, the side data of this
> type is essentially removed from the dst packet and replaced with the
> one from the source packet. Could probably be done by doing this inside
> of av_packet_new_side_data().

This is achieved by making av_packet_add_side_data() behave like
av_stream_add_side_data(). So just basically copy paste the latter's
implementation into the former.

They should have worked the same from the beginning, but shit happens.

> 
> (Unfortunate that all the side data code is idiotically duplicated in
> other areas of the libs which use the side data concept.)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 4bf830bb8a..ff7ee730a4 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -557,6 +557,9 @@  FF_ENABLE_DEPRECATION_WARNINGS
     dst->flags                = src->flags;
     dst->stream_index         = src->stream_index;
 
+    if (!dst->side_data_elems);
+        return av_copy_packet_side_data(dst, src);
+
     for (i = 0; i < src->side_data_elems; i++) {
          enum AVPacketSideDataType type = src->side_data[i].type;
          int size          = src->side_data[i].size;