[x264-devel] NAL slice support patch added?

[Loren Merritt]

On Mon, 22 Oct 2007, Mojtaba Hosseini wrote:

> 2) The current patch does not do anything specific for rate control. 
> Is this ok? That is, from the code, I do not see how this patch 
> could affect rate control but please advise if otherwise.

Now that slices are divorced from threads, ratecontrol shouldn't care 
about slices.

> 3) As per suggestion given on irc, number of NALs is allocated
> dynamically (because number of NALs in frame can potentially be
> very large). The default allocation is 20 and it grows by steps
> of 20, every time there is a need to grow. Is this a reasonable
> approach? Is 20 a good step size?

Does it even need to be >1 ? The arrays never shrink, so realloc is a O(1) 
cost over the whole movie. Otoh, if you did expect arrays large enough for 
copying to be nonnegligible, the usual way is to grow exponentially.

> 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?

How did you check for working? For patches that are supposed to change the 
output (thus regression testing can't work):
#define DEBUG_DUMP_FRAME.
Encode something with x264.
Decode it with JM.
Compare x264's fdec.yuv to JM's output.

The options to pay most attention to in this case are:
threads, 8x8dct, and intra partitions.

> 5) As some of you may know, ffmpeg has an interface for its h263+ and 
> mpeg4 encoders to specify number of bytes per 'slice' (started with 
> a resync marker). The interface then allows a callback function provided 
> by user to be called every time a 'slice' is made. The current 
> implementation of x264 would allow us to have a similar interface except 
> that user callback would be called at end of encoding process 
> (after x264_encoder_encoder() and when calling individual 
> x264_encode_NAL() functions). Is there opposition to adding 
> a callback function so that user can be called when a NAL is ready to 
> be encoded? Or is this too much change?

Ok if you can make it work. But with threading it can only reduce latency 
by 1 frame, it can't output slices as soon as they're encoded.
If it takes more than a few lines of code, make it a separate patch.

> --- common.h    (revision 680)
> +++ common.h    (working copy)
> @@ -53,7 +53,7 @@
>  #define X264_BFRAME_MAX 16
>  #define X264_THREAD_MAX 128
>  #define X264_SLICE_MAX 4
> -#define X264_NAL_MAX (4 + X264_SLICE_MAX)
> +#define X264_NAL_MAX (16 + X264_SLICE_MAX)

X264_NAL_MAX is no longer a maximum, and X264_SLICE_MAX should no longer 
exist.

> --- x264.h      (revision 680)
> +++ x264.h      (working copy)
> @@ -275,6 +275,7 @@
>      int b_aud;                  /* generate access unit delimiters */
>      int b_repeat_headers;       /* put SPS/PPS before each keyframe */
>      int i_sps_id;               /* SPS and PPS id number */
> +    int i_slice_max; /* maximum slice size in bytes, split slices if larger than this many bytes (0 => unlimited)  */
>  } x264_param_t;

max_slice_size is a better name, especially if you plan to add 
slice_count.
Also, it's not exactly a maximum, as a slicebreak occurs only after this
threshold is exceeded (by up to 385 bytes). This should be either fixed or
documented (I don't care which, but ffmpeg might).

> --- encoder.c   (revision 680)
> +++ encoder.c   (working copy)
> @@ -778,6 +779,18 @@
>      x264_nal_t *nal = &h->out.nal[h->out.i_nal];
>      nal->i_payload = &h->out.p_bitstream[bs_pos( &h->out.bs ) / 8] - nal->p_payload;
>      h->out.i_nal++;
> +    //if number of allocated nals is not enough, allocate a larger one (steps of X264_NAL_MAX)
> +    //and copy previous to new, using a temporary buffer.
> +    if(h->out.i_nal >= h->out.i_nals_allocated)
> +    {
> +        x264_nal_t *tmp  = x264_malloc(sizeof ( x264_nal_t) * h->out.i_nals_allocated);
> +        memcpy(tmp,h->out.nal,(sizeof ( x264_nal_t) * h->out.i_nals_allocated));
> +        x264_free(h->out.nal);
> +        h->out.nal = x264_malloc( sizeof ( x264_nal_t) * (h->out.i_nals_allocated + X264_NAL_MAX));
> +        memcpy(h->out.nal,tmp,(sizeof ( x264_nal_t) * h->out.i_nals_allocated));
> +        h->out.i_nals_allocated = h->out.i_nals_allocated + X264_NAL_MAX;
> +        x264_free(tmp);
> +    }
>  }

Overly complicated (only 1 alloc/copy is needed), and anyway should be 
in a x264_realloc().

--Loren Merritt