st/va: use provided sizes and coords for getimage

Submitted by Zhang, Boyuan on Oct. 5, 2018, 4:01 p.m.

Details

Message ID 1538755275-16849-1-git-send-email-boyuan.zhang@amd.com
State New
Headers show
Series "st/va: use provided sizes and coords for getimage" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Zhang, Boyuan Oct. 5, 2018, 4:01 p.m.
From: Boyuan Zhang <boyuan.zhang@amd.com>

vlVaGetImage should respect the width, height, and coordinates x and y that
passed in. Therefore, pipe_box should be created with the passed in values
instead of surface width/height. 

Signed-off-by: Boyuan Zhang <boyuan.zhang@amd.com>
---
 src/gallium/state_trackers/va/image.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/state_trackers/va/image.c b/src/gallium/state_trackers/va/image.c
index 3f892c9..c9f6f18 100644
--- a/src/gallium/state_trackers/va/image.c
+++ b/src/gallium/state_trackers/va/image.c
@@ -400,11 +400,14 @@  vlVaGetImage(VADriverContextP ctx, VASurfaceID surface, int x, int y,
    }
 
    for (i = 0; i < vaimage->num_planes; i++) {
-      unsigned width, height;
+      unsigned w = align(width, 2);
+      unsigned h = align(height, 2);
       if (!views[i]) continue;
-      vlVaVideoSurfaceSize(surf, i, &width, &height);
+      vl_video_buffer_adjust_size(&w, &h, i,
+                                  surf->templat.chroma_format,
+                                  surf->templat.interlaced);
       for (j = 0; j < views[i]->texture->array_size; ++j) {
-         struct pipe_box box = {0, 0, j, width, height, 1};
+         struct pipe_box box = {x, y, j, w, h, 1};
          struct pipe_transfer *transfer;
          uint8_t *map;
          map = drv->pipe->transfer_map(drv->pipe, views[i]->texture, 0,

Comments

This is an improvement, but I think you need to clip the box to

1. Size of the surface
2. Size of the image

I think that there are clipping helpers available to do that (maybe
pipe_box_clip or so? I forget, check the auxiliary dir). Christian -
does that make sense to you?

Cheers,

  -ilia
On Fri, Oct 5, 2018 at 12:01 PM <boyuan.zhang@amd.com> wrote:
>
> From: Boyuan Zhang <boyuan.zhang@amd.com>
>
> vlVaGetImage should respect the width, height, and coordinates x and y that
> passed in. Therefore, pipe_box should be created with the passed in values
> instead of surface width/height.
>
> Signed-off-by: Boyuan Zhang <boyuan.zhang@amd.com>
> ---
>  src/gallium/state_trackers/va/image.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/state_trackers/va/image.c b/src/gallium/state_trackers/va/image.c
> index 3f892c9..c9f6f18 100644
> --- a/src/gallium/state_trackers/va/image.c
> +++ b/src/gallium/state_trackers/va/image.c
> @@ -400,11 +400,14 @@ vlVaGetImage(VADriverContextP ctx, VASurfaceID surface, int x, int y,
>     }
>
>     for (i = 0; i < vaimage->num_planes; i++) {
> -      unsigned width, height;
> +      unsigned w = align(width, 2);
> +      unsigned h = align(height, 2);
>        if (!views[i]) continue;
> -      vlVaVideoSurfaceSize(surf, i, &width, &height);
> +      vl_video_buffer_adjust_size(&w, &h, i,
> +                                  surf->templat.chroma_format,
> +                                  surf->templat.interlaced);
>        for (j = 0; j < views[i]->texture->array_size; ++j) {
> -         struct pipe_box box = {0, 0, j, width, height, 1};
> +         struct pipe_box box = {x, y, j, w, h, 1};
>           struct pipe_transfer *transfer;
>           uint8_t *map;
>           map = drv->pipe->transfer_map(drv->pipe, views[i]->texture, 0,
> --
> 2.7.4
>
If that also fixes the problem, then yeah that makes perfect sense to me 
as well.

Christian.

Am 05.10.2018 um 18:11 schrieb Ilia Mirkin:
> This is an improvement, but I think you need to clip the box to
>
> 1. Size of the surface
> 2. Size of the image
>
> I think that there are clipping helpers available to do that (maybe
> pipe_box_clip or so? I forget, check the auxiliary dir). Christian -
> does that make sense to you?
>
> Cheers,
>
>    -ilia
> On Fri, Oct 5, 2018 at 12:01 PM <boyuan.zhang@amd.com> wrote:
>> From: Boyuan Zhang <boyuan.zhang@amd.com>
>>
>> vlVaGetImage should respect the width, height, and coordinates x and y that
>> passed in. Therefore, pipe_box should be created with the passed in values
>> instead of surface width/height.
>>
>> Signed-off-by: Boyuan Zhang <boyuan.zhang@amd.com>
>> ---
>>   src/gallium/state_trackers/va/image.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/va/image.c b/src/gallium/state_trackers/va/image.c
>> index 3f892c9..c9f6f18 100644
>> --- a/src/gallium/state_trackers/va/image.c
>> +++ b/src/gallium/state_trackers/va/image.c
>> @@ -400,11 +400,14 @@ vlVaGetImage(VADriverContextP ctx, VASurfaceID surface, int x, int y,
>>      }
>>
>>      for (i = 0; i < vaimage->num_planes; i++) {
>> -      unsigned width, height;
>> +      unsigned w = align(width, 2);
>> +      unsigned h = align(height, 2);
>>         if (!views[i]) continue;
>> -      vlVaVideoSurfaceSize(surf, i, &width, &height);
>> +      vl_video_buffer_adjust_size(&w, &h, i,
>> +                                  surf->templat.chroma_format,
>> +                                  surf->templat.interlaced);
>>         for (j = 0; j < views[i]->texture->array_size; ++j) {
>> -         struct pipe_box box = {0, 0, j, width, height, 1};
>> +         struct pipe_box box = {x, y, j, w, h, 1};
>>            struct pipe_transfer *transfer;
>>            uint8_t *map;
>>            map = drv->pipe->transfer_map(drv->pipe, views[i]->texture, 0,
>> --
>> 2.7.4
>>
Hi ilia,

I saw the function 
u_box_clip_2d(https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/auxiliary/util/u_box.h#n74).

But I still don't quite understand why we need to do this? Or say, what 
will happen if we don't do this box clipping here? Can you provide more 
information about this please?


Regards,

Boyuan


On 2018-10-05 12:11 PM, Ilia Mirkin wrote:
> This is an improvement, but I think you need to clip the box to
>
> 1. Size of the surface
> 2. Size of the image
>
> I think that there are clipping helpers available to do that (maybe
> pipe_box_clip or so? I forget, check the auxiliary dir). Christian -
> does that make sense to you?
>
> Cheers,
>
>    -ilia
> On Fri, Oct 5, 2018 at 12:01 PM <boyuan.zhang@amd.com> wrote:
>> From: Boyuan Zhang <boyuan.zhang@amd.com>
>>
>> vlVaGetImage should respect the width, height, and coordinates x and y that
>> passed in. Therefore, pipe_box should be created with the passed in values
>> instead of surface width/height.
>>
>> Signed-off-by: Boyuan Zhang <boyuan.zhang@amd.com>
>> ---
>>   src/gallium/state_trackers/va/image.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/va/image.c b/src/gallium/state_trackers/va/image.c
>> index 3f892c9..c9f6f18 100644
>> --- a/src/gallium/state_trackers/va/image.c
>> +++ b/src/gallium/state_trackers/va/image.c
>> @@ -400,11 +400,14 @@ vlVaGetImage(VADriverContextP ctx, VASurfaceID surface, int x, int y,
>>      }
>>
>>      for (i = 0; i < vaimage->num_planes; i++) {
>> -      unsigned width, height;
>> +      unsigned w = align(width, 2);
>> +      unsigned h = align(height, 2);
>>         if (!views[i]) continue;
>> -      vlVaVideoSurfaceSize(surf, i, &width, &height);
>> +      vl_video_buffer_adjust_size(&w, &h, i,
>> +                                  surf->templat.chroma_format,
>> +                                  surf->templat.interlaced);
>>         for (j = 0; j < views[i]->texture->array_size; ++j) {
>> -         struct pipe_box box = {0, 0, j, width, height, 1};
>> +         struct pipe_box box = {x, y, j, w, h, 1};
>>            struct pipe_transfer *transfer;
>>            uint8_t *map;
>>            map = drv->pipe->transfer_map(drv->pipe, views[i]->texture, 0,
>> --
>> 2.7.4
>>
On Tue, Oct 9, 2018 at 1:59 PM Boyuan Zhang <boyzhang@amd.com> wrote:
>
> Hi ilia,
>
> I saw the function
> u_box_clip_2d(https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/auxiliary/util/u_box.h#n74).
>
> But I still don't quite understand why we need to do this? Or say, what
> will happen if we don't do this box clipping here? Can you provide more
> information about this please?

Sure. Let's say you have:

1000x1000 video surface
500x500 image

Then you call

vlVaGetImage(surface, x=600, y=600, width=600, height=600, image)

Ideally you would retrieve the 400x400 "valid" area (from 600x600 at
the surface) and stick it into the image starting at 0,0.

Then let's say you have

1000x1000 video surface
100x100 image

Then you call

vlVaGetImage(surface, x=600, y=600, width=600, height=600, image)

Ideally the image would be filled with a 100x100 square from the
surface starting at 600x600 and ending at 700x700.

I haven't deeply dived into the VA docs. Perhaps some or all of these
are illegal. In which case the x/y/w/h need to be checked and errors
returned.

Cheers,

  -ilia
Thanks for the explanation ilia.

I'm curious too here that if it's legal for player to not respect the 
image size when calling vlVaGetImage. If player already know the size of 
image is 100x100, then why should it still call vlVaGetImage with 
width/height=600?

I mean when VA-API player calls to create image and create surface, it 
should behave itself, or will be considered a player bug. If player 
calls something out of range, even the driver have the size 
clipped(driver trying to fix player bug), but player still won't get 
expected stuff, since the requested size have been clipped by driver. 
Does this make sense?


Regards,

Boyuan


On 2018-10-09 02:10 PM, Ilia Mirkin wrote:
> On Tue, Oct 9, 2018 at 1:59 PM Boyuan Zhang <boyzhang@amd.com> wrote:
>> Hi ilia,
>>
>> I saw the function
>> u_box_clip_2d(https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/auxiliary/util/u_box.h#n74).
>>
>> But I still don't quite understand why we need to do this? Or say, what
>> will happen if we don't do this box clipping here? Can you provide more
>> information about this please?
> Sure. Let's say you have:
>
> 1000x1000 video surface
> 500x500 image
>
> Then you call
>
> vlVaGetImage(surface, x=600, y=600, width=600, height=600, image)
>
> Ideally you would retrieve the 400x400 "valid" area (from 600x600 at
> the surface) and stick it into the image starting at 0,0.
>
> Then let's say you have
>
> 1000x1000 video surface
> 100x100 image
>
> Then you call
>
> vlVaGetImage(surface, x=600, y=600, width=600, height=600, image)
>
> Ideally the image would be filled with a 100x100 square from the
> surface starting at 600x600 and ending at 700x700.
>
> I haven't deeply dived into the VA docs. Perhaps some or all of these
> are illegal. In which case the x/y/w/h need to be checked and errors
> returned.
>
> Cheers,
>
>    -ilia
On Tue, Oct 9, 2018 at 3:09 PM Boyuan Zhang <boyzhang@amd.com> wrote:
>
> Thanks for the explanation ilia.
>
> I'm curious too here that if it's legal for player to not respect the
> image size when calling vlVaGetImage. If player already know the size of
> image is 100x100, then why should it still call vlVaGetImage with
> width/height=600?
>
> I mean when VA-API player calls to create image and create surface, it
> should behave itself, or will be considered a player bug. If player
> calls something out of range, even the driver have the size
> clipped(driver trying to fix player bug), but player still won't get
> expected stuff, since the requested size have been clipped by driver.
> Does this make sense?

Yeah, that totally makes sense. Different APIs have different
approaches to such error conditions.

IMHO it'd be perfectly fine to return an error status in such cases,
in theory. But I'm no VA-API expert, and the docs[1] appear silent on
the matter.

Cheers,

  -ilia

[1] http://intel.github.io/libva/group__api__core.html#ga3d56f2eaf0be528a512cc935aca35418