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

[Diego Biurrun]

On Mon, Apr 09, 2007 at 12:52:32PM +0200, Michael Niedermayer wrote:
> 
> On Mon, Apr 09, 2007 at 11:39:45AM +0200, Reinhard Nissl wrote:
> > 
> > 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 ;-)
> 
> patch looks ok

Applied.

Diego