[FFmpeg-devel] [PATCH] avcodec/mpeg4videodec: Use more specific error codes

[James Almer]

On 3/10/2018 6:15 PM, Michael Niedermayer wrote:
> On Sat, Mar 10, 2018 at 03:33:33PM -0300, James Almer wrote:
>> On 3/10/2018 2:34 PM, Michael Niedermayer wrote:
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>  libavcodec/mpeg4videodec.c | 100 +++++++++++++++++++++++----------------------
>>>  1 file changed, 51 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
>>> index 19210d97fe..1357b357a8 100644
>>> --- a/libavcodec/mpeg4videodec.c
>>> +++ b/libavcodec/mpeg4videodec.c
>>> @@ -448,7 +448,7 @@ int ff_mpeg4_decode_video_packet_header(Mpeg4DecContext *ctx)
>>>  
>>>      /* is there enough space left for a video packet + header */
>>>      if (get_bits_count(&s->gb) > s->gb.size_in_bits - 20)
>>> -        return -1;
>>> +        return AVERROR_INVALIDDATA;
>>>  
>>>      for (len = 0; len < 32; len++)
>>>          if (get_bits1(&s->gb))
>>> @@ -456,7 +456,7 @@ int ff_mpeg4_decode_video_packet_header(Mpeg4DecContext *ctx)
>>>  
>>>      if (len != ff_mpeg4_get_video_packet_prefix_length(s)) {
>>>          av_log(s->avctx, AV_LOG_ERROR, "marker does not match f_code\n");
>>> -        return -1;
>>> +        return AVERROR_INVALIDDATA;
>>>      }
>>>  
>>>      if (ctx->shape != RECT_SHAPE) {
>>> @@ -468,7 +468,7 @@ int ff_mpeg4_decode_video_packet_header(Mpeg4DecContext *ctx)
>>>      if (mb_num >= s->mb_num || !mb_num) {
>>>          av_log(s->avctx, AV_LOG_ERROR,
>>>                 "illegal mb_num in video packet (%d %d) \n", mb_num, s->mb_num);
>>> -        return -1;
>>> +        return AVERROR_INVALIDDATA;
>>>      }
>>>  
>>>      s->mb_x = mb_num % s->mb_width;
>>> @@ -597,7 +597,7 @@ static inline int mpeg4_decode_dc(MpegEncContext *s, int n, int *dir_ptr)
>>>  
>>>      if (code < 0 || code > 9 /* && s->nbit < 9 */) {
>>>          av_log(s->avctx, AV_LOG_ERROR, "illegal dc vlc\n");
>>> -        return -1;
>>> +        return AVERROR_INVALIDDATA;
>>>      }
>>>  
>>>      if (code == 0) {
>>> @@ -620,7 +620,7 @@ static inline int mpeg4_decode_dc(MpegEncContext *s, int n, int *dir_ptr)
>>>              if (get_bits1(&s->gb) == 0) { /* marker */
>>>                  if (s->avctx->err_recognition & (AV_EF_BITSTREAM|AV_EF_COMPLIANT)) {
>>>                      av_log(s->avctx, AV_LOG_ERROR, "dc marker bit missing\n");
>>> -                    return -1;
>>> +                    return AVERROR_INVALIDDATA;
>>>                  }
>>>              }
>>>          }
>>> @@ -664,7 +664,7 @@ static int mpeg4_decode_partition_a(Mpeg4DecContext *ctx)
>>>                      if (cbpc < 0) {
>>>                          av_log(s->avctx, AV_LOG_ERROR,
>>>                                 "mcbpc corrupted at %d %d\n", s->mb_x, s->mb_y);
>>> -                        return -1;
>>> +                        return cbpc;
>>
>> get_vlc2() seems to return -1 on error, so nothing really changes with
>> this.
> 
> right, ill hardcode these
> 
> 
>> Same with every other similar call, 
> 
> the other calls should return proper error codes and we should
> forward these.
> Not forwarding an error because the current implementation of a
> function generates an error code that the caller can hardcode is
> bad design.
> We should not duplicate the implementation of what a function returns
> in the caller. The implementation could change and then this is wrong.
> 
> Or am i missing a reason why hardcoding the error codes in the caller
> would be an advantage ?

I just meant to return AVERROR_INVALIDDATA on every other check for
get_vlc2() return value, much like the one i pointed above, since those
would also try to return -1 otherwise.

> 
> will send a new patch
> 
> thx
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>