[Mesa-dev] [PATCH] st/va: add mpeg4 startcode workaround

[Zhang, Boyuan]

Hi Emil,

Please see the following information about this patch.

- Issue: For Mpeg4, the VOP and GOV headers were truncated. With the existing workaround in st/va, playback shows massive corruptions.
- This Patch: Provide another way to get the truncated headers back. Massive corruptions are gone with this patch. At the same time, add an environmental variable to allow user to decide whether to use this patch.

Regards,
Boyuan

-----Original Message-----
From: Liu, Leo 
Sent: November-05-15 10:15 PM
To: Emil Velikov; Zhang, Boyuan
Cc: ML mesa-dev
Subject: RE: [Mesa-dev] [PATCH] st/va: add mpeg4 startcode workaround

+Boyuan, forgot to Cc him when I sent.

Regards,
Leo


>-----Original Message-----
>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>Sent: Thursday, November 05, 2015 7:00 PM
>To: Liu, Leo
>Cc: ML mesa-dev
>Subject: Re: [Mesa-dev] [PATCH] st/va: add mpeg4 startcode workaround
>
>On 5 November 2015 at 21:00, Leo Liu <leo.liu at amd.com> wrote:
>> From: Boyuan Zhang <boyuan.zhang at amd.com>
>>
>> Signed-off-by: Boyuan Zhang <boyuan.zhang at amd.com>
>> Reviewed-by: Christian König <christian.koenig at amd.com>
>> ---
>>  src/gallium/state_trackers/va/buffer.c     |  24 +++++-
>>  src/gallium/state_trackers/va/context.c    |   7 ++
>>  src/gallium/state_trackers/va/picture.c    | 117 +++++++++++++++++------------
>>  src/gallium/state_trackers/va/va_private.h |   3 +
>>  4 files changed, 102 insertions(+), 49 deletions(-)
>>
>Guys can get some commit message please ? What is the workaround why is 
>it needed ?
>
>It's a bit sad that one has to ask for such a thing.
>
>
>> @@ -59,8 +70,17 @@ vlVaCreateBuffer(VADriverContextP ctx, VAContextID
>context, VABufferType type,
>>        return VA_STATUS_ERROR_ALLOCATION_FAILED;
>>     }
>>
>> -   if (data)
>> -      memcpy(buf->data, data, size * num_elements);
>> +   uint8_t* pExternalData = (uint8_t*) data;
>> +   if (data) {
>> +      if ((u_reduce_video_profile(pContext->desc.base.profile) ==
>PIPE_VIDEO_FORMAT_MPEG4)
>> +            && (pContext->mpeg4.vaapi_mpeg4_workaround == true)
>> +            && (buf->type == VASliceDataBufferType)) {
>Please follow st/va coding style - drop the explicit comparison against 
>true, && should be at the end of the line.
>
>
>> --- a/src/gallium/state_trackers/va/context.c
>> +++ b/src/gallium/state_trackers/va/context.c
>> @@ -35,6 +35,8 @@
>>
>>  #include "va_private.h"
>>
>> +DEBUG_GET_ONCE_BOOL_OPTION(mpeg4, "VAAPI_MPEG4_WORKAROUND",
>FALSE);
>> +
>You do realise that defined as it, one can only use 
>VAAPI_MPEG4_WORKAROUND on debug mesa builds ?
>
>
>> @@ -275,6 +277,11 @@ vlVaCreateContext(VADriverContextP ctx, 
>> VAConfigID
>config_id, int picture_width,
>>              return VA_STATUS_ERROR_ALLOCATION_FAILED;
>>           }
>>        }
>> +
>> +      if (u_reduce_video_profile(context->decoder->profile) ==
>> +            PIPE_VIDEO_FORMAT_MPEG4) {
>u_reduce_video_profile() is called three times already. Stash it into a 
>local variable and keep the comparison on a single line ?
>
>
>> --- a/src/gallium/state_trackers/va/picture.c
>> +++ b/src/gallium/state_trackers/va/picture.c
>> @@ -584,60 +584,83 @@ vlVaDecoderFixMPEG4Startcode(vlVaContext
>> *context)
>
>> +      for (int i = 0 ; i < VL_VA_MPEG4_BYTES_FOR_LOOKUP ; i++) {
>> +         if (memcmp (p, start_code, sizeof(start_code)) == 0) {
>> +            found = true;
>Just use startcode_available directly ?
>
>> +            break;
>> +         }
>> +         p += 1;
>> +         extraSize += 1;
>> +      }
>> +      if (found) {
>> +         startcode_available = true;
>
>
>-Emil