[x264-devel] NAL slice support patch added?

[Kurosu]

Hi,

Mojtaba Hosseini a écrit :
> 4) I have tested this patch with multiple threads and it worked.
> Is there a set of tests I should do to increase confidence in the
> patch?

A generic strangeness I've found with a quite similar patch is that the
number of threads has an influence on the content of the slice (in terms
of contained MBs). But the same happens with or without slices. The
rate-distortion curve is almost identical for long sequences, so I guess
this difference in behavior is benign (and could have been expected).

Comments below are applied to fake inlined patch, so I've split a bit
the lines.

> +        i_nal_size += (bs_pos(&h->out.bs) + x264_cabac_pos(&h->cabac)
> +                    - mb_spos);
> +        if ((mb_xy >= h->sh.i_last_mb)
> +            || ((h->param.i_slice_max > 0) &&
> +                (i_nal_size>(h->param.i_slice_max<<3))))

Nitpick: why not initialize "i_nal_size" to h->param.i_slice_max<<3,
then _decrement_ it by the amount of bits generated and test for
i_nal_size<0 (simpler test). It would then be renamed to i_bits_left or
whatever.

I see 2 more important problems with this though:
- You stop the slice once it has *already* overflown the limit, so from
a packetization perspective, you're bound to create fragments which is
probably not what anyone want; this is hard to address, except by
estimating the coding cost of the next MB, or being able to cancel its
encoding
- nitpick, but you don't account for any syntax closing the slice
(trailing bits, ...); annexe B additional start codes don't apply
though, as they're probably going to be stripped

> +    h->stat.frame.i_misc_bits = bs_pos( &h->out.bs )
> +                              + NALU_OVERHEAD * 8

That should be (out.i_nal-startnal-1)*NALU_OVERHEAD * 8, I guess.

> +    for (i=startnal; i<h->out.i_nal; i++)
> +    {
> +        i_frame_size += h->out.nal[i].i_payload;
> +    }

Nitpick, this could be done in the while loop above.

Best regards,
-- 
Kurosu