[FFmpeg-devel] avcodec/h264_parse: Treat escaped and unescaped decoding error equal in decode_extradata_ps_mp4()

Submitted by Michael Niedermayer on Nov. 25, 2017, 9:49 p.m.

Details

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

Commit Message

Michael Niedermayer Nov. 25, 2017, 9:49 p.m.
Fixes: lorex.mp4

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/h264_parse.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Clément Bœsch Nov. 25, 2017, 10:08 p.m.
On Sat, Nov 25, 2017 at 10:49:09PM +0100, Michael Niedermayer wrote:
> Fixes: lorex.mp4
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/h264_parse.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
> index a7c71d9bbb..9216d0bdbd 100644
> --- a/libavcodec/h264_parse.c
> +++ b/libavcodec/h264_parse.c
> @@ -427,8 +427,6 @@ static int decode_extradata_ps_mp4(const uint8_t *buf, int buf_size, H264ParamSe
>  
>          ret = decode_extradata_ps(escaped_buf, escaped_buf_size, ps, 1, logctx);
>          av_freep(&escaped_buf);
> -        if (ret < 0)
> -            return ret;

If you don't want the return code to be reintroduced differently 10x in
the future (like, someone deciding to return ret in the function instead
of 0), I'd suggest 2 things:

- use "(void)decode_extradata_ps(...)" to explicitly ignore the code
  return; it's a hint for the compiler and the developer, typically used
  in OpenBSD (I believe that's because they warn about unchecked return
  code by default)
- add a comment above about the why

No comment on the change itself.
Carl Eugen Hoyos Nov. 25, 2017, 11:58 p.m.
2017-11-25 22:49 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> Fixes: lorex.mp4

Please mention ticket #6762 if it is related.

Carl Eugen
Michael Niedermayer Dec. 3, 2017, 12:57 a.m.
On Sun, Nov 26, 2017 at 12:58:45AM +0100, Carl Eugen Hoyos wrote:
> 2017-11-25 22:49 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> > Fixes: lorex.mp4
> 
> Please mention ticket #6762 if it is related.

yes, it seems a shorter variant of the same file that i worked with

will add ticket #6762
thx

[...]
Michael Niedermayer Dec. 3, 2017, 1:02 a.m.
On Sat, Nov 25, 2017 at 11:08:46PM +0100, Clément Bœsch wrote:
> On Sat, Nov 25, 2017 at 10:49:09PM +0100, Michael Niedermayer wrote:
> > Fixes: lorex.mp4
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/h264_parse.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
> > index a7c71d9bbb..9216d0bdbd 100644
> > --- a/libavcodec/h264_parse.c
> > +++ b/libavcodec/h264_parse.c
> > @@ -427,8 +427,6 @@ static int decode_extradata_ps_mp4(const uint8_t *buf, int buf_size, H264ParamSe
> >  
> >          ret = decode_extradata_ps(escaped_buf, escaped_buf_size, ps, 1, logctx);
> >          av_freep(&escaped_buf);
> > -        if (ret < 0)
> > -            return ret;
> 
> If you don't want the return code to be reintroduced differently 10x in
> the future (like, someone deciding to return ret in the function instead
> of 0), I'd suggest 2 things:
> 
> - use "(void)decode_extradata_ps(...)" to explicitly ignore the code
>   return; it's a hint for the compiler and the developer, typically used
>   in OpenBSD (I believe that's because they warn about unchecked return
>   code by default)
> - add a comment above about the why

will push with these changes

thx

[...]
Alexander Agranovsky Dec. 15, 2017, 5:30 p.m.
Would it be possible to cherry-pick into n3.4?

Thanks!


On 12/2/17 20:02, Michael Niedermayer wrote:
> On Sat, Nov 25, 2017 at 11:08:46PM +0100, Clément Bœsch wrote:
>> On Sat, Nov 25, 2017 at 10:49:09PM +0100, Michael Niedermayer wrote:
>>> Fixes: lorex.mp4
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>   libavcodec/h264_parse.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
>>> index a7c71d9bbb..9216d0bdbd 100644
>>> --- a/libavcodec/h264_parse.c
>>> +++ b/libavcodec/h264_parse.c
>>> @@ -427,8 +427,6 @@ static int decode_extradata_ps_mp4(const uint8_t *buf, int buf_size, H264ParamSe
>>>   
>>>           ret = decode_extradata_ps(escaped_buf, escaped_buf_size, ps, 1, logctx);
>>>           av_freep(&escaped_buf);
>>> -        if (ret < 0)
>>> -            return ret;
>> If you don't want the return code to be reintroduced differently 10x in
>> the future (like, someone deciding to return ret in the function instead
>> of 0), I'd suggest 2 things:
>>
>> - use "(void)decode_extradata_ps(...)" to explicitly ignore the code
>>    return; it's a hint for the compiler and the developer, typically used
>>    in OpenBSD (I believe that's because they warn about unchecked return
>>    code by default)
>> - add a comment above about the why
> will push with these changes
>
> thx
>
> [...]
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer Dec. 15, 2017, 7:28 p.m.
On Fri, Dec 15, 2017 at 12:30:15PM -0500, Alexander Agranovsky wrote:
> Would it be possible to cherry-pick into n3.4?

ill backport it to release/3.4

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
index a7c71d9bbb..9216d0bdbd 100644
--- a/libavcodec/h264_parse.c
+++ b/libavcodec/h264_parse.c
@@ -427,8 +427,6 @@  static int decode_extradata_ps_mp4(const uint8_t *buf, int buf_size, H264ParamSe
 
         ret = decode_extradata_ps(escaped_buf, escaped_buf_size, ps, 1, logctx);
         av_freep(&escaped_buf);
-        if (ret < 0)
-            return ret;
     }
 
     return 0;