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

2016-06-02 Thread Julien Isorce 
no problem, it was actually good, I think that even if current code is
correct, I'll still submit
the new diff for review because it looks better.

On 2 June 2016 at 10:34, Eric Engestrom  wrote:

> On Tue, May 31, 2016 at 02:54:06PM +0200, Christian König wrote:
> > 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.
>
> I guess the lesson for me is to not try and fix stuff in code I don't
> understand at all :]
>
> Sorry for wasting your time with this.
>
> Cheers,
>   Eric
>
> >
> > 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,
> > > > >templat);
> > > > +  templat.buffer_format = format;
> > > > +  tmp_buf = drv->pipe->create_video_buffer(drv->pipe, );
> > > >
> > > >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  > > > > wrote:
> > > >
> > > > Am 31.05.2016 um 03:24 schrieb Eric Engestrom:
> > > >
> > > > CoverityID: 1337953
> > > >
> > > > Signed-off-by: Eric Engestrom 
> > > > ---
> > > >
> > > > 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@lists.freedesktop.org
> > > > 
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> > > >
> > >
> >
>
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-02 Thread Eric Engestrom 
On Tue, May 31, 2016 at 02:54:06PM +0200, Christian König wrote:
> 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.

I guess the lesson for me is to not try and fix stuff in code I don't
understand at all :]

Sorry for wasting your time with this.

Cheers,
  Eric

> 
> 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,
> > > >templat);
> > > +  templat.buffer_format = format;
> > > +  tmp_buf = drv->pipe->create_video_buffer(drv->pipe, );
> > > 
> > >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  > > > wrote:
> > > 
> > > Am 31.05.2016 um 03:24 schrieb Eric Engestrom:
> > > 
> > > CoverityID: 1337953
> > > 
> > > Signed-off-by: Eric Engestrom 
> > > ---
> > > 
> > > 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@lists.freedesktop.org
> > > 
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > 
> > > 
> > 
> 

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

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

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

2016-05-31 Thread 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, 
>templat);

+  templat.buffer_format = format;
+  tmp_buf = drv->pipe->create_video_buffer(drv->pipe, );

   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 > wrote:


Am 31.05.2016 um 03:24 schrieb Eric Engestrom:

CoverityID: 1337953

Signed-off-by: Eric Engestrom 
---

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@lists.freedesktop.org

https://lists.freedesktop.org/mailman/listinfo/mesa-dev






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

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

2016-05-31 Thread 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, 
>templat);

+  templat.buffer_format = format;
+  tmp_buf = drv->pipe->create_video_buffer(drv->pipe, );

   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 > wrote:


Am 31.05.2016 um 03:24 schrieb Eric Engestrom:

CoverityID: 1337953

Signed-off-by: Eric Engestrom >
---

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@lists.freedesktop.org 
https://lists.freedesktop.org/mailman/listinfo/mesa-dev




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

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

2016-05-31 Thread 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, >templat);
+  templat.buffer_format = format;
+  tmp_buf = drv->pipe->create_video_buffer(drv->pipe, );

   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  wrote:

> Am 31.05.2016 um 03:24 schrieb Eric Engestrom:
>
>> CoverityID: 1337953
>>
>> Signed-off-by: Eric Engestrom 
>> ---
>>
>> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-05-31 Thread Christian König 

Am 31.05.2016 um 03:24 schrieb Eric Engestrom:

CoverityID: 1337953

Signed-off-by: Eric Engestrom 
---

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-05-30 Thread Eric Engestrom 
CoverityID: 1337953

Signed-off-by: Eric Engestrom 
---

Note that I do not know this code at all; I'm blindly following Coverity's 
advice on this one :]

---
 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;
}
-- 
2.8.3

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