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

[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 <eric.engestrom at imgtec.com> 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,
> > > > &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
> > > >
> > > >
> > >
> >
>
> > _______________________________________________
> > mesa-dev mailing list
> > 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/20160602/c1c5cc19/attachment-0001.html>