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

[James Almer]

On 4/5/2017 6:53 PM, Hendrik Leppkes wrote:
> On Wed, Apr 5, 2017 at 11:40 PM, James Almer <jamrial at gmail.com> wrote:
>> 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.
>>
> 
> Keeping the current behavior for the HEVC software decoder seems
> ideal. If a change in behavior is wanted afterwards, it should be
> dealt with in a separate change, and not this refactoring.
> 
> - Hendrik

Patchset pushed then.

Thanks.