[x264-devel] Re: Patch for Scenarist HD compatibility

[Loren Merritt]

On Sat, 6 Jan 2007, Ian Caulfield wrote:

> - fix a small bug in bitstream_write (on x86 a right shift of more than 32
> bits actually shifts by the shift amount mod 32, rather than returning 0)
>
> --- common/bs.h (revision 616)
> +++ common/bs.h (working copy)
> @@ -185,7 +185,10 @@
>  }
>  else
>  {
> -     *s->p = (*s->p << s->i_left) | (i_bits >> (i_count - s->i_left));
> +     if( (i_count - s->i_left) >= 32 )
> +         *s->p = (*s->p << s->i_left);
> +     else
> +         *s->p = (*s->p << s->i_left) | (i_bits >> (i_count - s->i_left));
>       i_count -= s->i_left;
>       s->p++;
>       s->i_left = 8;

Not a bug. i_count is the number of bits passed in i_bits, which is a 
uint32_t, thus i_count<=32. i_left>0 because whenever it is 0 we increment 
the bitstream pointer and refill it. So the shift can't exceed 31.

bs_align_10 doesn't look right. shouldn't you use bs_rbsp_trailing 
instead?

> - move the version info SEI header after the SPS and PPS headers 
> (required for Scenarist to identify the stream as an H264 ES)

Bugs should be fixed where the bug is. This is a bug in Scenarist, so out 
of principle I won't work around it in x264.


On Sun, 7 Jan 2007, bond wrote:

> one question: why is there the need to explicitely enable writing the
> HRD nal info?  wouldnt it be better to simply always write this when VBV
> is used?

The standard says: if nal_hrd_parameters is present, then the SEIs must 
also be present. I wouldn't mind it if just the nal_hrd_parameters could 
be tied to the usage of VBV, but the SEIs impose a measurable bitrate 
overhead so they should not be automatically enabled.

> also wouldnt it be better to let the user instead enable/disable whether
> he wants the SEIs in the bitstream? they might be useful or not wanted
> independant of whether VBV is used

These SEIs don't contain any information other than about VBV, so it
would just be wrong to put them in a non-VBV-compliant stream.

> or HDDVD compliance is wanted...

The whole patch is independent of whether HDDVD compliance is wanted. It
just adds a feature that Scenarist requires, because Scenarist is too
lazy to implement its own VBV model. (hint: a smart muxer does _not_
need to know what VBV parameters the stream was encoded with, it only
needs to know what VBV parameters the decoder wants)

--Loren Merritt

-- 
This is the x264-devel mailing-list
To unsubscribe, go to: http://developers.videolan.org/lists.html