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

[Michael Niedermayer]

Hi

On Mon, Apr 09, 2007 at 11:39:45AM +0200, Reinhard Nissl wrote:
> 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 ;-)

patch looks ok

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070409/97a4915b/attachment.pgp>