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

2015-11-08 Thread Ilia Mirkin 
On Sun, Nov 8, 2015 at 6:41 AM, Christian König  wrote:
> On 06.11.2015 20:28, Ilia Mirkin wrote:
>>
>> On Fri, Nov 6, 2015 at 2:15 PM, Christian König 
>> wrote:
>>>
>>> On 06.11.2015 20:10, Ilia Mirkin wrote:

 On Fri, Nov 6, 2015 at 1:48 PM, Zhang, Boyuan 
 wrote:
>
> 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.

 Why would the user not want to use this? Sounds like a correctness
 fix, no? Or is it some thing that a hypothetical gallium driver might
 not need but the radeon uvd-based ones do? In that case it should be
 behind a PIPE_VIDEO_CAP_bla (sorry, I'm still not too clear on what
 "bla" is here...)
>>>
>>>
>>> The problem is that this is a rather extreme hack.
>>>
>>> As you probably knew VA-API didn't correctly specify which start code
>>> should
>>> be included and which shouldn't for MPEG-4. This is an issue for AMD as
>>> well
>>> as NVidia hardware and pretty much everybody which sticks close to an
>>> elementary stream.
>>>
>>> What we do in this hack is just searching the bytes *before* the pointer
>>> and
>>> size we got from the application for the stuff that's missing. E.g. we
>>> access memory the application didn't told us to access.
>>>
>>> This is rather speculative, but works surprisingly well with a lot of
>>> applications.
>>
>> Hm, that is a little dodgy indeed. But making user-selectable options
>> (provided via env var) for correct decoding... doesn't seem ideal
>> either. Is there some "correct" way to resolve this without changing
>> the va api?
>
>
> Unfortunately no. I wasn't involved in everything but we had a couple of
> people working on this which have more knowledge about MPEG-4 part 2 than
> me.
>
> A couple of month back somebody from a different team at AMD even tried to
> convince Intel to fix this, but as far as I know without success.
>
> The over all conclusion is that the interface definition of VA-API for
> MPEG-4 part 2 is just a bloody mess.

I see. Since everything I knew about MPEG4 has been forgotten many
years ago (and I've never figured out the va api in the first place),
I'll take the above as a given. So it sounds like there are a few
options --

(a) enable workaround by default, provide a way to disable
(b) no workaround by default, provide a way to enable, people have
buggy rendering with no great way to find out about the enable
(c) no workaround by default, don't expose MPEG4 in the va endpoint.
provide a way to enable workaround, which in turn enables MPEG4

I kinda like (c). That way end users don't get broken rendering, and
ones who really want to use va-api for mpeg4 can enable it if they
know what they're doing. What do you think?

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-08 Thread Emil Velikov 
On 8 November 2015 at 11:41, Christian König  wrote:
> On 06.11.2015 20:28, Ilia Mirkin wrote:
>>
>> On Fri, Nov 6, 2015 at 2:15 PM, Christian König 
>> wrote:
>>>
>>> On 06.11.2015 20:10, Ilia Mirkin wrote:

 On Fri, Nov 6, 2015 at 1:48 PM, Zhang, Boyuan 
 wrote:
>
> 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.

 Why would the user not want to use this? Sounds like a correctness
 fix, no? Or is it some thing that a hypothetical gallium driver might
 not need but the radeon uvd-based ones do? In that case it should be
 behind a PIPE_VIDEO_CAP_bla (sorry, I'm still not too clear on what
 "bla" is here...)
>>>
>>>
>>> The problem is that this is a rather extreme hack.
>>>
>>> As you probably knew VA-API didn't correctly specify which start code
>>> should
>>> be included and which shouldn't for MPEG-4. This is an issue for AMD as
>>> well
>>> as NVidia hardware and pretty much everybody which sticks close to an
>>> elementary stream.
>>>
>>> What we do in this hack is just searching the bytes *before* the pointer
>>> and
>>> size we got from the application for the stuff that's missing. E.g. we
>>> access memory the application didn't told us to access.
>>>
>>> This is rather speculative, but works surprisingly well with a lot of
>>> applications.
>>
>> Hm, that is a little dodgy indeed. But making user-selectable options
>> (provided via env var) for correct decoding... doesn't seem ideal
>> either. Is there some "correct" way to resolve this without changing
>> the va api?
>
>
> Unfortunately no. I wasn't involved in everything but we had a couple of
> people working on this which have more knowledge about MPEG-4 part 2 than
> me.
>
> A couple of month back somebody from a different team at AMD even tried to
> convince Intel to fix this, but as far as I know without success.
>
> The over all conclusion is that the interface definition of VA-API for
> MPEG-4 part 2 is just a bloody mess.
>
And precisely for the above reasons, I keep on going on like an old
hag to keep writing something in the commit logs.



Adding workarounds is fine, as long as they are documented - what it
does (if not obvious), why we need it and even references when
available. Otherwise the next person will just come and remove this
code and/or do something that completely breaks things up, as there is
no information that can prevent (educate) them from doing do.



Cheers
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-08 Thread Christian König 

On 06.11.2015 20:28, Ilia Mirkin wrote:

On Fri, Nov 6, 2015 at 2:15 PM, Christian König  wrote:

On 06.11.2015 20:10, Ilia Mirkin wrote:

On Fri, Nov 6, 2015 at 1:48 PM, Zhang, Boyuan 
wrote:

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.

Why would the user not want to use this? Sounds like a correctness
fix, no? Or is it some thing that a hypothetical gallium driver might
not need but the radeon uvd-based ones do? In that case it should be
behind a PIPE_VIDEO_CAP_bla (sorry, I'm still not too clear on what
"bla" is here...)


The problem is that this is a rather extreme hack.

As you probably knew VA-API didn't correctly specify which start code should
be included and which shouldn't for MPEG-4. This is an issue for AMD as well
as NVidia hardware and pretty much everybody which sticks close to an
elementary stream.

What we do in this hack is just searching the bytes *before* the pointer and
size we got from the application for the stuff that's missing. E.g. we
access memory the application didn't told us to access.

This is rather speculative, but works surprisingly well with a lot of
applications.

Hm, that is a little dodgy indeed. But making user-selectable options
(provided via env var) for correct decoding... doesn't seem ideal
either. Is there some "correct" way to resolve this without changing
the va api?


Unfortunately no. I wasn't involved in everything but we had a couple of 
people working on this which have more knowledge about MPEG-4 part 2 
than me.


A couple of month back somebody from a different team at AMD even tried 
to convince Intel to fix this, but as far as I know without success.


The over all conclusion is that the interface definition of VA-API for 
MPEG-4 part 2 is just a bloody mess.


Regards,
Christian.



   -ilia


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-06 Thread Ilia Mirkin 
On Fri, Nov 6, 2015 at 1:48 PM, Zhang, Boyuan  wrote:
> 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.

Why would the user not want to use this? Sounds like a correctness
fix, no? Or is it some thing that a hypothetical gallium driver might
not need but the radeon uvd-based ones do? In that case it should be
behind a PIPE_VIDEO_CAP_bla (sorry, I'm still not too clear on what
"bla" is here...)

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-06 Thread Christian König 

On 06.11.2015 20:10, Ilia Mirkin wrote:

On Fri, Nov 6, 2015 at 1:48 PM, Zhang, Boyuan  wrote:

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.

Why would the user not want to use this? Sounds like a correctness
fix, no? Or is it some thing that a hypothetical gallium driver might
not need but the radeon uvd-based ones do? In that case it should be
behind a PIPE_VIDEO_CAP_bla (sorry, I'm still not too clear on what
"bla" is here...)


The problem is that this is a rather extreme hack.

As you probably knew VA-API didn't correctly specify which start code 
should be included and which shouldn't for MPEG-4. This is an issue for 
AMD as well as NVidia hardware and pretty much everybody which sticks 
close to an elementary stream.


What we do in this hack is just searching the bytes *before* the pointer 
and size we got from the application for the stuff that's missing. E.g. 
we access memory the application didn't told us to access.


This is rather speculative, but works surprisingly well with a lot of 
applications.


Regards,
Christian.



   -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-06 Thread Ilia Mirkin 
On Fri, Nov 6, 2015 at 2:15 PM, Christian König  wrote:
> On 06.11.2015 20:10, Ilia Mirkin wrote:
>>
>> On Fri, Nov 6, 2015 at 1:48 PM, Zhang, Boyuan 
>> wrote:
>>>
>>> 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.
>>
>> Why would the user not want to use this? Sounds like a correctness
>> fix, no? Or is it some thing that a hypothetical gallium driver might
>> not need but the radeon uvd-based ones do? In that case it should be
>> behind a PIPE_VIDEO_CAP_bla (sorry, I'm still not too clear on what
>> "bla" is here...)
>
>
> The problem is that this is a rather extreme hack.
>
> As you probably knew VA-API didn't correctly specify which start code should
> be included and which shouldn't for MPEG-4. This is an issue for AMD as well
> as NVidia hardware and pretty much everybody which sticks close to an
> elementary stream.
>
> What we do in this hack is just searching the bytes *before* the pointer and
> size we got from the application for the stuff that's missing. E.g. we
> access memory the application didn't told us to access.
>
> This is rather speculative, but works surprisingly well with a lot of
> applications.

Hm, that is a little dodgy indeed. But making user-selectable options
(provided via env var) for correct decoding... doesn't seem ideal
either. Is there some "correct" way to resolve this without changing
the va api?

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-06 Thread 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.veli...@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@amd.com> wrote:
>> From: Boyuan Zhang <boyuan.zh...@amd.com>
>>
>> Signed-off-by: Boyuan Zhang <boyuan.zh...@amd.com>
>> Reviewed-by: Christian König <christian.koe...@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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-05 Thread Emil Velikov 
On 5 November 2015 at 21:00, Leo Liu  wrote:
> From: Boyuan Zhang 
>
> Signed-off-by: Boyuan Zhang 
> Reviewed-by: Christian König 
> ---
>  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-05 Thread Leo Liu 
From: Boyuan Zhang 

Signed-off-by: Boyuan Zhang 
Reviewed-by: Christian König 
---
 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(-)

diff --git a/src/gallium/state_trackers/va/buffer.c 
b/src/gallium/state_trackers/va/buffer.c
index 71a6503..e1f4c31 100644
--- a/src/gallium/state_trackers/va/buffer.c
+++ b/src/gallium/state_trackers/va/buffer.c
@@ -30,6 +30,7 @@
 #include "state_tracker/drm_driver.h"
 #include "util/u_memory.h"
 #include "util/u_handle_table.h"
+#include "util/u_video.h"
 #include "util/u_transfer.h"
 #include "vl/vl_winsys.h"
 
@@ -41,10 +42,20 @@ vlVaCreateBuffer(VADriverContextP ctx, VAContextID context, 
VABufferType type,
  VABufferID *buf_id)
 {
vlVaBuffer *buf;
+   vlVaDriver *drv;
+   vlVaContext *pContext;
 
if (!ctx)
   return VA_STATUS_ERROR_INVALID_CONTEXT;
 
+   drv = VL_VA_DRIVER(ctx);
+   if (!drv)
+  return VA_STATUS_ERROR_INVALID_CONTEXT;
+
+   pContext = handle_table_get(drv->htab, context);
+   if (!pContext)
+  return VA_STATUS_ERROR_INVALID_CONTEXT;
+
buf = CALLOC(1, sizeof(vlVaBuffer));
if (!buf)
   return VA_STATUS_ERROR_ALLOCATION_FAILED;
@@ -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)) {
+ memcpy(pContext->mpeg4.extra_data,
+pExternalData - VL_VA_MPEG4_BYTES_FOR_LOOKUP,
+VL_VA_MPEG4_BYTES_FOR_LOOKUP);
+  }
+  memcpy(buf->data, pExternalData, buf->size * buf->num_elements);
+   }
 
*buf_id = handle_table_add(VL_VA_DRIVER(ctx)->htab, buf);
 
diff --git a/src/gallium/state_trackers/va/context.c 
b/src/gallium/state_trackers/va/context.c
index 845b547..45240f5 100644
--- 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);
+
 #include 
 
 static struct VADriverVTable vtable =
@@ -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) {
+ context->mpeg4.vaapi_mpeg4_workaround = debug_get_option_mpeg4();
+  }
}
 
context->desc.base.profile = config_id;
diff --git a/src/gallium/state_trackers/va/picture.c 
b/src/gallium/state_trackers/va/picture.c
index e850689..1f1b427 100644
--- a/src/gallium/state_trackers/va/picture.c
+++ b/src/gallium/state_trackers/va/picture.c
@@ -584,60 +584,83 @@ vlVaDecoderFixMPEG4Startcode(vlVaContext *context)
int mod_time;
unsigned int vop_size;
unsigned int vop_coding_type = context->desc.mpeg4.vop_coding_type;
+   bool startcode_available = false;
 
context->mpeg4.start_code_size = 0;
memset(context->mpeg4.start_code, 0, sizeof(context->mpeg4.start_code));
-   if (vop_coding_type+1 == PIPE_MPEG12_PICTURE_CODING_TYPE_I) {
-  unsigned int vop_time = context->mpeg4.frame_num/
-context->desc.mpeg4.vop_time_increment_resolution;
-  unsigned int vop_hour = vop_time / 3600;
-  unsigned int vop_minute = (vop_time / 60) % 60;
-  unsigned int vop_second = vop_time % 60;
-  uint8_t group_of_vop[] = { 0x00, 0x00, 0x01, 0xb3, 0x00, 0x00, 0x00 };
-  struct bit_stream bs_gvop = {group_of_vop, sizeof(group_of_vop)*8, 32};
-
-  write_bits(_gvop, vop_hour, 5);
-  write_bits(_gvop, vop_minute, 6);
-  write_bit(_gvop, 1); /* marker_bit */
-  write_bits(_gvop, vop_second, 6);
-  write_bit(_gvop, 0); /* closed_gov */ /* TODO replace magic */
-  write_bit(_gvop, 0); /* broken_link */
-  write_bit(_gvop, 0); /* padding */
-  write_bits(_gvop, 7, 3); /* padding */
-
-  memcpy(context->mpeg4.start_code, group_of_vop, sizeof(group_of_vop));
-  context->mpeg4.start_code_size += sizeof(group_of_vop);
+   if (context->mpeg4.vaapi_mpeg4_workaround == true) {
+  uint8_t* p = (uint8_t*) context->mpeg4.extra_data;
+  const uint8_t start_code[] = { 0x00, 0x00, 0x01, 0xb6 };
+  int extraSize = 0;
+  bool found = false;
+  for (int i = 0 ; i < VL_VA_MPEG4_BYTES_FOR_LOOKUP ; i++) {
+ if (memcmp (p, start_code,

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

2015-11-05 Thread Liu, Leo 
+Boyuan, forgot to Cc him when I sent.

Regards,
Leo


>-Original Message-
>From: Emil Velikov [mailto:emil.l.veli...@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@amd.com> wrote:
>> From: Boyuan Zhang <boyuan.zh...@amd.com>
>>
>> Signed-off-by: Boyuan Zhang <boyuan.zh...@amd.com>
>> Reviewed-by: Christian König <christian.koe...@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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev