[Mesa-dev] [PATCH] st/va: remove assert for single slice

[Christian König]

Sorry for the delay, as noted in the other mail I was on sick leave for 
a while.

Am 03.02.2017 um 05:52 schrieb Nayan Deshmukh:
> On Thu, Feb 2, 2017 at 3:34 PM, Christian König <deathsimple at vodafone.de> wrote:
>> Am 01.02.2017 um 13:59 schrieb Nayan Deshmukh:
>>> we anyway allow for multiple slices
>>>
>>> Signed-off-by: Nayan Deshmukh <nayan26deshmukh at gmail.com>
>>> ---
>>>    src/gallium/state_trackers/va/picture_mpeg12.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>>> diff --git a/src/gallium/state_trackers/va/picture_mpeg12.c
>>> b/src/gallium/state_trackers/va/picture_mpeg12.c
>>> index 812e9e5..4d9c45f 100644
>>> --- a/src/gallium/state_trackers/va/picture_mpeg12.c
>>> +++ b/src/gallium/state_trackers/va/picture_mpeg12.c
>>> @@ -81,6 +81,5 @@ void vlVaHandleIQMatrixBufferMPEG12(vlVaContext
>>> *context, vlVaBuffer *buf)
>>>      void vlVaHandleSliceParameterBufferMPEG12(vlVaContext *context,
>>> vlVaBuffer *buf)
>>>    {
>>> -   assert(buf->size >= sizeof(VASliceParameterBufferMPEG2) &&
>>> buf->num_elements == 1);
>>
>> NAK, we can lower the requirements here, but we shouldn't completely remove
>> the assert.
>>
>> Especially since we ignore all of the fields from the
>> VASliceParameterBufferMPEG2 structure and so won't catch errors.
>>
> I agree, we should have some assert here.
>
>> Please test something like the following:
>>
>> assert(buf->size >= (sizeof(VASliceParameterBufferMPEG2) *
>> buf->num_elements));
>>
> buf->size represents the size of a single buffer element and hence it
> will be equal to sizeof(VASliceParameterBufferMPEG2). So we can have
> something like
>
> assert(buf->size >= sizeof(VASliceParameterBufferMPEG2))

Sounds like a good idea to me, please send that out as a patch.

>
> I also tried using the fields of the VASliceParameterBufferMPEG2. I
> misunderstood
> the code first, as mpv sends all the slices at ones and buf->data points to
> num_elements number of VASliceParameterBuffers. I tried using this
> size fields with the slice data buffers, but surprisingly it leads to no change.
> Any ideas why? I am attaching the diff for reference.

Could be that the shader based decoder has a bug with multiple slices as 
well.

Another possibility is that the other slices are missing the start code.

Do you have a screenshoot of the problem? And maybe a dump of what is in 
the VASliceParameterBufferMPEG2 buffers and in your bitstream buffer?

Regards,
Christian.

>
> Which means that bug https://bugs.freedesktop.org/show_bug.cgi?id=93760
> is still open.
>
> Regards,
> Nayan
>
> diff --git a/src/gallium/state_trackers/va/picture.c
> b/src/gallium/state_trackers/va/picture.c
> index 82584ea..8e68e35 100644
> --- a/src/gallium/state_trackers/va/picture.c
> +++ b/src/gallium/state_trackers/va/picture.c
> @@ -262,8 +262,10 @@ handleVASliceDataBufferType(vlVaContext *context,
> vlVaBuffer *buf)
>   {
>      enum pipe_video_format format;
>      unsigned num_buffers = 0;
> -   void * const *buffers[2];
> -   unsigned sizes[2];
> +   void * const *buffers[context->desc.mpeg12.num_slices + 1];
> +   unsigned sizes[context->desc.mpeg12.num_slices + 1];
> +   char *cur;
> +   int i;
>      static const uint8_t start_code_h264[] = { 0x00, 0x00, 0x01 };
>      static const uint8_t start_code_h265[] = { 0x00, 0x00, 0x01 };
>      static const uint8_t start_code_vc1[] = { 0x00, 0x00, 0x01, 0x0d };
> @@ -306,9 +308,12 @@ handleVASliceDataBufferType(vlVaContext *context,
> vlVaBuffer *buf)
>         break;
>      }
>
> -   buffers[num_buffers] = buf->data;
> -   sizes[num_buffers] = buf->size;
> -   ++num_buffers;
> +   cur = buf->data;
> +   for (i = 0; i < context->desc.mpeg12.num_slices; ++i) {
> +      buffers[i] = cur;
> +      cur = cur + context->sizes[i];
> +   }
> +
>
>      if (context->needs_begin_frame) {
>         context->decoder->begin_frame(context->decoder, context->target,
> @@ -316,7 +321,8 @@ handleVASliceDataBufferType(vlVaContext *context,
> vlVaBuffer *buf)
>         context->needs_begin_frame = false;
>      }
>      context->decoder->decode_bitstream(context->decoder,
> context->target, &context->desc.base,
> -      num_buffers, (const void * const*)buffers, sizes);
> +      context->desc.mpeg12.num_slices, (const void * const*)buffers,
> context->sizes);
> +
>   }
>
>   static VAStatus
> @@ -586,6 +592,7 @@ vlVaEndPicture(VADriverContextP ctx, VAContextID context_id)
>      }
>
>      context->decoder->end_frame(context->decoder, context->target,
> &context->desc.base);
> +   FREE(context->sizes);
>      if (context->decoder->entrypoint == PIPE_VIDEO_ENTRYPOINT_ENCODE) {
>         int idr_period = context->desc.h264enc.gop_size / context->gop_coeff;
>         int p_remain_in_idr = idr_period - context->desc.h264enc.frame_num;
> diff --git a/src/gallium/state_trackers/va/picture_mpeg12.c
> b/src/gallium/state_trackers/va/picture_mpeg12.c
> index 4d9c45f..9e51274 100644
> --- a/src/gallium/state_trackers/va/picture_mpeg12.c
> +++ b/src/gallium/state_trackers/va/picture_mpeg12.c
> @@ -81,5 +81,16 @@ void vlVaHandleIQMatrixBufferMPEG12(vlVaContext
> *context, vlVaBuffer *buf)
>
>   void vlVaHandleSliceParameterBufferMPEG12(vlVaContext *context,
> vlVaBuffer *buf)
>   {
> +   VASliceParameterBufferMPEG2 *mpeg2 = buf->data;
> +   unsigned *sizes = MALLOC(buf->num_elements * sizeof(unsigned));
> +   int i;
> +
>      context->desc.mpeg12.num_slices += buf->num_elements;
> +
> +   for(i = 0; i < buf->num_elements; ++i) {
> +      sizes[i] = mpeg2->slice_data_size;
> +      mpeg2 = mpeg2 + 1;
> +   }
> +
> +   context->sizes = sizes;
>   }
> diff --git a/src/gallium/state_trackers/va/va_private.h
> b/src/gallium/state_trackers/va/va_private.h
> index a744461..47cb65e 100644
> --- a/src/gallium/state_trackers/va/va_private.h
> +++ b/src/gallium/state_trackers/va/va_private.h
> @@ -263,6 +263,7 @@ typedef struct {
>      bool first_single_submitted;
>      int gop_coeff;
>      bool needs_begin_frame;
> +   unsigned *sizes;
>   } vlVaContext;
>
>   typedef struct {