[Mesa-dev] [PATCH] st/va: remove assert for single slice
Nayan Deshmukh
nayan26deshmukh at gmail.com
Fri Feb 10 13:44:29 UTC 2017
On Fri, Feb 10, 2017 at 5:09 PM, Christian König
<deathsimple at vodafone.de> wrote:
> 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.
>
It could be. Andy can you test it with your hardware decoder to see if
its specific
to shader decoder.
> Another possibility is that the other slices are missing the start code.
Do we need to add any specific start code for MPEG2 vids as we don't add
anything in the va state tracker?
>
> Do you have a screenshoot of the problem? And maybe a dump of what is in the
> VASliceParameterBufferMPEG2 buffers and in your bitstream buffer?
>
Here is the link for the video that I am using;
https://drive.google.com/drive/folders/0BxP5-S1t9VEEbkR4dWhTUFozV2s
Regards,
Nayan
> 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 {
>
>
>
More information about the mesa-dev
mailing list