[Ffmpeg-devel] h264: fix processing of end of sequence nal unit

[Reinhard Nissl]

Hi,

Michael Niedermayer wrote:

> On Sun, Apr 08, 2007 at 10:31:18PM +0200, Reinhard Nissl wrote:
>
>> without the attached patch, a correctly decoded still image which ends
>> in an end of sequence nal unit was not delivered but dropped.
> 
> 1. describe problem (ok)
> 2. provide enough information to reproduce the problem (missing)

Thought that the above two lines were sufficient. You need a properly
coded frame (e. g. for an "I" frame, SPS, PPS, Slice) and after the
frame there shall be a end of sequence NAL unit, i. e. the four bytes 00
00 01 0a.

> 3. analyze the problem (missing and optional but essential before 4.)

I'll keep this in mind for further patches. Thanks for your patience.

When decode_nal() decodes the end of sequence NAL unit, it returns with
dst_length == 0. The original code leads to a return -1 which discards
the current properly decoded frame. So the first change is necessary to
go on to the later following switch where nal_type_code is handled.

The next change (which you declared unrelated below) is necessary as
dst_length may now be 0, so it's not a good idea to first access ptr[-1]
and then check for dst_length beeing > 1. For performance reasons, this
change could be omitted as ptr[-1] points to valid memory which is never
0 (at least as long 00 00 01 00 doesn't appear in a valid stream).

But I think, this loop should run up to dst_length > 0. If I understand
this loop correctly, it is used to skip trailing 00 which are actually
part of the next NAL unit (i. e. consider something like 00 00 01 0a 00
00 00 01 ..., the spec says (among other things) that the current NAL
unit is to be terminated by a sequence of 00 00 00), but it currently
misses to skip such a byte when the actual length of rbsp is 0.

Meanwhile I understand why you declared it unrelated. So I've removed it
from the patch and created a separate thread for this issue.

Regarding the last change, it is only used to set bit_length to 0 as it
might get negative when decode_rbsp_trailing() accesses ptr[-1]. This
change can be omitted for performance reasons too, as init_get_bits()
takes care of negative bit_sizes.

> 4. send patch (ok)

So regarding performance and just this issue, the remaining patch
(attached) is rather small ;-)

> my choice now is reverse engeneer what your patch does, and guess why
> it doesnt work without this change and then decide based on this if the
> change is correct or alternative i reject it
> 
> considering that you provide no information to reproduce the problem
> so i cannot test it at all i have no choice but to reject it anyway
> 
> [...]
>> -        while(ptr[dst_length - 1] == 0 && dst_length > 1)
>> +        while(dst_length > 0 && ptr[dst_length - 1] == 0)
> 
> unrelated changes

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl at gmx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-h264-fix-end-of-sequence-updated.patch
Type: text/x-patch
Size: 463 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070409/2be0ccd2/attachment.bin>