On 1/8/07, <b class="gmail_sendername">Loren Merritt</b> <<a href="mailto:lorenm@u.washington.edu">lorenm@u.washington.edu</a>> wrote:<div><span class="gmail_quote"></span><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
On Sat, 6 Jan 2007, Ian Caulfield wrote:<br><br>> - fix a small bug in bitstream_write (on x86 a right shift of more than 32<br>> bits actually shifts by the shift amount mod 32, rather than returning 0)<br>><br>
> --- common/bs.h (revision 616)<br>> +++ common/bs.h (working copy)<br>> @@ -185,7 +185,10 @@<br>> }<br>> else<br>> {<br>> - *s->p = (*s->p << s->i_left) | (i_bits >> (i_count - s->i_left));
<br>> + if( (i_count - s->i_left) >= 32 )<br>> + *s->p = (*s->p << s->i_left);<br>> + else<br>> + *s->p = (*s->p << s->i_left) | (i_bits >> (i_count - s->i_left));
<br>> i_count -= s->i_left;<br>> s->p++;<br>> s->i_left = 8;<br><br>Not a bug. i_count is the number of bits passed in i_bits, which is a<br>uint32_t, thus i_count<=32. i_left>0 because whenever it is 0 we increment
<br>the bitstream pointer and refill it. So the shift can't exceed 31.</blockquote><div><br>Sorry, I should have explained further - I had the problem when bs_write_ue was being called with a value having 23 significant bits - bs_write was then being called with a bit count of 45. I assumed that the intended behaviour was to zero-extend the value when the count exceeded 31. I could modify the patch to make bs_write_ue make two calls to bs_write if you'd prefer.
<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">bs_align_10 doesn't look right. shouldn't you use bs_rbsp_trailing<br>instead?
</blockquote><div><br>The difference is the one bit is optional - if the end of the SEI payload is byte aligned, nothing further is written. From the (draft) spec:<br><br><span style="font-family: courier new,monospace;">
sei_payload( payloadType, payloadSize )<br>{<br> <snip payload contents><br> if( !byte_aligned() )<br> bit_equal_to_one<br> while ( !byte_aligned() )<br> bit_equal_to_zero<br>}<br style="font-family: courier new,monospace;">
</span> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">> - move the version info SEI header after the SPS and PPS headers<br>> (required for Scenarist to identify the stream as an H264 ES)
<br><br>Bugs should be fixed where the bug is. This is a bug in Scenarist, so out<br>of principle I won't work around it in x264.</blockquote><div><br>Could it be a requirement of the HD DVD spec that H264 streams start with AUD, SPS, PPS? I don't have access to that spec...
<br><br>I did consider adding a flag to remove the version header from the bitstream, but when I found that reordering was sufficient I thought that just patching that would be fairly unobtrusive<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
On Sun, 7 Jan 2007, bond wrote:</blockquote><blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote">> or HDDVD compliance is wanted...<br></blockquote>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>The whole patch is independent of whether HDDVD compliance is wanted. It<br>just adds a feature that Scenarist requires, because Scenarist is too
<br>lazy to implement its own VBV model. (hint: a smart muxer does _not_<br>need to know what VBV parameters the stream was encoded with, it only<br>needs to know what VBV parameters the decoder wants)</blockquote><div><br>
Interestingly enough, Scenarist doesn't require the presence of VBV parameters in VC-1 streams<br><br>Ian<br></div><br></div><br>