[Mesa-dev] [PATCH] st/va: restore old buffer format on error

[Christian König]

On the other hand the original code was correct in the first place.

And as Julien noted Coverity tries to restore the template buffer format 
from a local variable which doesn't exists any more where you wanted to 
add the line.

So Coverity creates code which won't compile and needs to be fixed instead.

Christian.

Am 31.05.2016 um 14:51 schrieb Christian König:
> Yeah, that solution looks more correct to me.
>
> Christian.
>
> Am 31.05.2016 um 14:44 schrieb Julien Isorce:
>> Hi,
>> Thx for looking at it but are you sure your diff compiles ?
>>
>> Can you try this instead:
>>
>> --- a/src/gallium/state_trackers/va/image.c
>> +++ b/src/gallium/state_trackers/va/image.c
>> @@ -471,19 +471,19 @@ vlVaPutImage(VADriverContextP ctx, VASurfaceID 
>> surface, VAImageID image,
>>
>>     if (format != surf->buffer->buffer_format) {
>>        struct pipe_video_buffer *tmp_buf;
>> -      enum pipe_format old_surf_format = surf->templat.buffer_format;
>> +      struct pipe_video_buffer templat = surf->templat;
>>
>> -      surf->templat.buffer_format = format;
>> -      tmp_buf = drv->pipe->create_video_buffer(drv->pipe, 
>> &surf->templat);
>> +      templat.buffer_format = format;
>> +      tmp_buf = drv->pipe->create_video_buffer(drv->pipe, &templat);
>>
>>        if (!tmp_buf) {
>> -         surf->templat.buffer_format = old_surf_format;
>>           pipe_mutex_unlock(drv->mutex);
>>           return VA_STATUS_ERROR_ALLOCATION_FAILED;
>>        }
>>
>>        surf->buffer->destroy(surf->buffer);
>>        surf->buffer = tmp_buf;
>> +      surf->templat.buffer_format = format;
>>     }
>>
>> Cheers
>> Julien
>>
>>
>> On 31 May 2016 at 08:43, Christian König <christian.koenig at amd.com 
>> <mailto:christian.koenig at amd.com>> wrote:
>>
>>     Am 31.05.2016 um 03:24 schrieb Eric Engestrom:
>>
>>         CoverityID: 1337953
>>
>>         Signed-off-by: Eric Engestrom <eric at engestrom.ch>
>>         ---
>>
>>         Note that I do not know this code at all; I'm blindly
>>         following Coverity's advice on this one :]
>>
>>
>>     Well and that is completely nonsense. The buffer was already
>>     reallocated when this error happens and so resetting the template
>>     to the original value is incorrect and actually rather dangerous.
>>
>>     Why does Coverity things that we should add this? And how can we
>>     fix this?
>>
>>     Regards,
>>     Christian.
>>
>>
>>         ---
>>           src/gallium/state_trackers/va/image.c | 1 +
>>           1 file changed, 1 insertion(+)
>>
>>         diff --git a/src/gallium/state_trackers/va/image.c
>>         b/src/gallium/state_trackers/va/image.c
>>         index 92d014c..8cfe17a 100644
>>         --- a/src/gallium/state_trackers/va/image.c
>>         +++ b/src/gallium/state_trackers/va/image.c
>>         @@ -490,6 +490,7 @@ vlVaPutImage(VADriverContextP ctx,
>>         VASurfaceID surface, VAImageID image,
>>                views =
>>         surf->buffer->get_sampler_view_planes(surf->buffer);
>>              if (!views) {
>>         +      surf->templat.buffer_format = old_surf_format;
>>                 pipe_mutex_unlock(drv->mutex);
>>                 return VA_STATUS_ERROR_OPERATION_FAILED;
>>              }
>>
>>
>>     _______________________________________________
>>     mesa-dev mailing list
>>     mesa-dev at lists.freedesktop.org
>>     <mailto:mesa-dev at lists.freedesktop.org>
>>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160531/7f7d2fe1/attachment.html>