[FFmpeg-devel] [PATCH 4/4] lavc/cbs_h2645: fix no slice data trigger the assert.

[mypopy at gmail.com]

2018-05-11 23:38 GMT+08:00 James Almer <jamrial at gmail.com>:
> On 5/11/2018 7:10 AM, Mark Thompson wrote:
>> On 11/05/18 06:11, Jun Zhao wrote:
>>> when the NALU data with zero, just give a warning.
>>>
>>> Fixes ticket #7200
>>>
>>> Signed-off-by: Jun Zhao <mypopydev at gmail.com>
>>> ---
>>>  libavcodec/cbs_h2645.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index ab33cdb..08b060c 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>          // Remove trailing zeroes.
>>>          while (size > 0 && nal->data[size - 1] == 0)
>>>              --size;
>>> -        av_assert0(size > 0);
>>> +        if (!size) {
>>> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
>>> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
>>> +            continue;
>>> +        }
>>>
>>>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>          if (!data)
>>>
>>
>> What do we actually want the result to be here?
>>
>> On IRC, James suggested:
>>
>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>> index dbf2435677..d436d65f48 100644
>>> --- a/libavcodec/h2645_parse.c
>>> +++ b/libavcodec/h2645_parse.c
>>> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>              ret = hevc_parse_nal_header(nal, logctx);
>>>          else
>>>              ret = h264_parse_nal_header(nal, logctx);
>>> -        if (ret <= 0 || nal->size <= 0) {
>>> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>>>              if (ret < 0) {
>>>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>>>                         nal->type);
>>
>> which removes it before it gets to the CBS code.
>>
>> Another thing we could do is:
>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index ab33cdb69b..46cd887cdd 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>          uint8_t *data;
>>>
>>>          // Remove trailing zeroes.
>>> -        while (size > 0 && nal->data[size - 1] == 0)
>>> +        while (size > 1 && nal->data[size - 1] == 0)
>>>              --size;
>>>          av_assert0(size > 0);
>>>
>>
>> which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.
>>
>> So, what do you think?  Do you know what made your sample stream?
>>
>> - Mark
>
> Taking into account the analysis by mkver in the trac ticket, where he
> found out the bitstream contains "00 00 00 01 00 00 00 01" with the
> second start code being a real valid NAL, i think this should definitely
> be fixed in h2645_parse. No reason to propagate a non existent NAL like
> this.
> We should either use my fix, or another that actually prevents nal->size
> from inexplicably becoming 1 in this scenario.
Now h2645_parse give a loose check in ff_h2645_packet_split(), and I agree
use your fix
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel