| Message ID | 20170503032151.25225-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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...
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; >
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 [...]
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.
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 [...]
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 [...]
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 [...]
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.
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
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 [...]
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? [...]
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
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 >
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.
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. [...]
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.
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?
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 >
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 ? [...]
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 [...]
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.
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.)
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 >
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;
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(+)