[Mesa-dev] [PATCH 1/2] st/vdpau: fix vlVdpOutputSurfaceRender(Output|Bitmap)Surface

[Emil Velikov]

On 26/08/14 18:01, Ilia Mirkin wrote:
> On Tue, Aug 26, 2014 at 12:50 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 15/08/14 19:33, Emil Velikov wrote:
>>> On 14/08/14 10:59, Christian König wrote:
>>>> From: Christian König <christian.koenig at amd.com>
>>>>
>>>> Correctly handle that the source_surface is only optional.
>>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80561
>>> Cc: mesa-stable at lists.freedesktop.org
>>>
>>> Would be nice to get this in stable :)
>>>
>> Hi Christian,
>>
>> Did you miss my comments below, or am I miss-understanding the spec ?
>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>  src/gallium/state_trackers/vdpau/device.c        | 43 +++++++++++++++++++++++-
>>>>  src/gallium/state_trackers/vdpau/output.c        | 42 +++++++++++++++--------
>>>>  src/gallium/state_trackers/vdpau/vdpau_private.h |  1 +
>>>>  3 files changed, 71 insertions(+), 15 deletions(-)
>>>>
>> [snip]
>>>> diff --git a/src/gallium/state_trackers/vdpau/output.c b/src/gallium/state_trackers/vdpau/output.c
>>>> index caae50f..3248f76 100644
>>>> --- a/src/gallium/state_trackers/vdpau/output.c
>>>> +++ b/src/gallium/state_trackers/vdpau/output.c
>>>> @@ -639,12 +639,19 @@ vlVdpOutputSurfaceRenderOutputSurface(VdpOutputSurface destination_surface,
>>>>     if (!dst_vlsurface)
>>>>        return VDP_STATUS_INVALID_HANDLE;
>>>>
>>>> -   src_vlsurface = vlGetDataHTAB(source_surface);
>>>> -   if (!src_vlsurface)
>>>> -      return VDP_STATUS_INVALID_HANDLE;
>>>> +   if (source_surface == VDP_INVALID_HANDLE) {
>>> AFAICS the spec says "If source_surface is NULL..." whereas VDP_INVALID_HANDLE
>>> is defined as 0xffffffffU.
>>>
>> Here
> 
> typedef uint32_t VdpOutputSurface
> 
> It doesn't make sense to check for NULL... I think that
> VDP_INVALID_HANDLE is in the same spirit as 'NULL'. Although perhaps
> they really meant 0 (is 0 some sort of otherwise-disallowed handle?).
> Not sure what the nvidia vdpau libs do...
> 
I'm inclined to go with if (!source_surface), and judging by the reporter
quotation of the spec, I guess that they'll do the same. Whereas for what
exact is meant in the spec & design, that would be an interesting question
indeed :)

I'll bring it up with on the vdpau ML.

-Emil

>   -ilia
> 
>>
>>>> +      src_sv = dst_vlsurface->device->dummy_sv;
>>>> +
>>>> +   } else {
>>>> +      vlVdpOutputSurface *src_vlsurface = vlGetDataHTAB(source_surface);
>>>> +      if (!src_vlsurface)
>>>> +         return VDP_STATUS_INVALID_HANDLE;
>>>>
>>>> -   if (dst_vlsurface->device != src_vlsurface->device)
>>>> -      return VDP_STATUS_HANDLE_DEVICE_MISMATCH;
>>>> +      if (dst_vlsurface->device != src_vlsurface->device)
>>>> +         return VDP_STATUS_HANDLE_DEVICE_MISMATCH;
>>>> +
>>>> +      src_sv = src_vlsurface->sampler_view;
>>>> +   }
>>>>
>>>>     pipe_mutex_lock(dst_vlsurface->device->mutex);
>>>>     vlVdpResolveDelayedRendering(dst_vlsurface->device, NULL, NULL);
>>>> @@ -703,12 +710,19 @@ vlVdpOutputSurfaceRenderBitmapSurface(VdpOutputSurface destination_surface,
>>>>     if (!dst_vlsurface)
>>>>        return VDP_STATUS_INVALID_HANDLE;
>>>>
>>>> -   src_vlsurface = vlGetDataHTAB(source_surface);
>>>> -   if (!src_vlsurface)
>>>> -      return VDP_STATUS_INVALID_HANDLE;
>>>> +   if (source_surface == VDP_INVALID_HANDLE) {
>>> Ditto.
>>>
>> and here
>>
>> Cheers,
>> Emil
>>
>>> I'm not sure if we correctly handle another case from the spec (see below) but
>>> that can be tackled independently.
>>>
>>>
>>> "The surface is treated as having four components: red, green, blue and alpha.
>>> Any missing components are treated as 1.0. For example, for an A8
>>> VdpBitmapSurface, alpha will come from the surface but red, green and blue
>>> will be treated as 1.0"
>>>
>>>
>>> With the two fixed, the patch
>>> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
>>>
>>> Thanks
>>> Emil
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev