[XviD-devel] [Fwd: Data partitioning for encoder]

[sigdrak]

Hello,

Skal a écrit :
> 	i've applied your code for error handling. Definitely cleaner.

I should have made a separate patch. But again, as I went on with code, 
it was getting difficult to separate stuff.

> 	Not only API: your new log2bin() func taken from ffmpeg ...
> 	wasn't equivalent to the current one. Anyway, i've committed
> 	a faster version (not that it's critical func, but still...).
> 	Xvid_bench.c has a bench for that, now.
Well, yes I was so lame as to remove the old code :)

Anyway, log2_tab_16 is only 16-elements-long, and not 256 (or else you 
remove the last test):
http://www.xvid.org/cvs/chora/co.php/xvidcore/src/bitstream/bitstream.c?login=2&r=1.51
Plus I don't see the benefit (yet) of having it global to this file.


> 	Also, for you and Angela Belda (working on resync markers):
> 	i've added your functions BitstreamWriteEndOfSequence(),
> 	BitstreamWriteGroupOfVopHeader()

Remnant of an unproper patch extraction (code was heavily edited to 
fit)... Anyway, that reminds me: IIRC, XviD generates an Video Object 
Sequence plus an Video Object Layer at the GOV boundaries instead of the 
expectable GOV. Why that ?

> 	at the end of bitstream.c, since they looked ok. 
> 	They are *unused* as of now, but now you have them handy...
> 	(@Sigdrak: your patch did declare BitStreamWriteStartOfVideoPacket(), 
> 	but the implementation was missing)

Yes I tried to separate things to make them easier (ahem) to check. I've 
just made it a PITA...

Do you suggest that I propose new patches on this basis? I think I would 
start with VideoPacket this time, as it comes first hierarchically speaking.

Btw, I've noticed that DP and Interlacing aren't supported together. I 
don't know how that should be handled in XviD framework (erroring out?)

> 	Please check,

Probably only aesthetical, but I find:
--8<--
if (frame->coding_type != I_VOP)
         addbits = frame->fcode -1;
if (frame->coding_type == B_VOP)
         addbits = MAX(frame->fcode, frame->bcode)-1;
-->8--
difficult to read. My suggestion if you want to avoid the switch case:
--8<--
if (frame->coding_type == P_VOP)
         addbits = frame->fcode -1;
else if (frame->coding_type == B_VOP)
         addbits = MAX(frame->fcode, frame->bcode)-1;
-->8--
Maybe your assembly knowledge/check justifies otherwise.

Then the standard (14496-2:2001, I don't have 2003 handy right now) states:
"resync_marker:  This is a binary string of at least 16 zero’s followed 
by a one ‘0 0000 0000 0000 0001’. For an I-VOP or a VOP where 
video_object_layer_shape has the value “binary_only”, the resync marker 
is 16 zeros followed by a one."
For I, NUM bits, OK

"The length of this resync marker is dependent on the value of 
vop_fcode_forward, for a P-VOP or a S(GMC)-VOP[...]. For a P-VOP and a 
S(GMC)-VOP, the resync_marker is (15+fcode) zeros followed by a one"
So, for P, 15+fcode+1=NUM+fcode-1, OK

"[...] and the larger value of either vop_fcode_forward and 
vop_fcode_backward for a B-VOP [...]for a B-VOP , the resync_marker is 
max(15+fcode,17) zeros followed by a one."
I don't know how is bcode is computed, so your code looks suspicious to 
me, provided 2003 doesn't modify this.

BTW, "A resync marker shall only be located immediately before a 
macroblock and aligned with a byte."
So Angela should be able to split at VP boundaries.

decoder.c diff seems fine to me.

That's all the changes my CVS abilities allowed me to track and confirm.

Cordially,
sigdrak