[FFmpeg-devel] [PATCH 1/5] avcodec/hevc_parse: check for parameter set decoding failure

[James Almer]

On 4/3/2017 10:46 AM, James Almer wrote:
> On 4/3/2017 7:00 AM, Michael Niedermayer wrote:
>> On Sun, Apr 02, 2017 at 10:45:41PM -0300, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++-------
>>>  1 file changed, 25 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c
>>> index 6c1138e015..028ca5afe5 100644
>>> --- a/libavcodec/hevc_parse.c
>>> +++ b/libavcodec/hevc_parse.c
>>> @@ -22,7 +22,8 @@
>>>  #include "hevc_parse.h"
>>>  
>>>  static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps,
>>> -                                 int is_nalff, int nal_length_size, void *logctx)
>>> +                                 int is_nalff, int nal_length_size, int err_recognition,
>>> +                                 void *logctx)
>>>  {
>>>      int i;
>>>      int ret = 0;
>>> @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets
>>>  
>>>          /* ignore everything except parameter sets and VCL NALUs */
>>>          switch (nal->type) {
>>> -        case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps);    break;
>>> -        case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break;
>>> -        case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps);    break;
>>> +        case HEVC_NAL_VPS:
>>> +            ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps);
>>> +            if (ret < 0)
>>> +                goto done;
>>> +            break;
>>> +        case HEVC_NAL_SPS:
>>> +            ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1);
>>> +            if (ret < 0)
>>> +                goto done;
>>> +            break;
>>> +        case HEVC_NAL_PPS:
>>> +            ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps);
>>> +            if (ret < 0)
>>> +                goto done;
>>> +            break;
>>>          case HEVC_NAL_TRAIL_R:
>>>          case HEVC_NAL_TRAIL_N:
>>>          case HEVC_NAL_TSA_N:
>>
>> I didnt investigate how exactly this is used but from just the patch
>> this seems not optimal
>> For example, if you have 3 PPS NALs and the first fails to decode
>> you might still be able to fully decode the other 2
> 
> I'm mimicking the behavior of decode_nal_unit() in hevcdec.c, which is
> currently used during frame decoding and extradata decoding.
> This patchset aims at removing duplicate code while keeping the hevc
> decoder behaving the same as it was before. I could skip this change
> if that's preferred, but if this behavior is not optimal then someone
> who better understands the codec should look at it.

To add some context, the functions in hevc_parse.c are currently used
only by the mediacodec based hevc decoder to decode extradata, and it's
a duplicate of existing functionality in hevcdec.c used by the internal
hevc decoder.

This set aims at putting the hevc_parse.c version on par with the
hevcdec.c one (in the scope of extradata parsing, not frame) to share it
between the two decoders and any other that may need it in the future.
This patch checks for PS failures and aborts if err_recog is set to
explode. The second makes apply_defdispwin user defined instead of
having it hardcoded to 1. The third avoids aborting on NALs not needed
or expected in extradata, and the last two patches make hevcdec.c use
ff_hevc_decode_extradata().

Not aborting on PS NAL failures here would technically mean a change in
behavior on the internal hevc decoder once patch five is applied, and
I'd rather no do that as part of this set.