[vlc-devel] [PATCH] picture: copy picture context in picture_Copy() (fixes #14456)

[Rémi Denis-Courmont]

Le maanantaina 5. kesäkuuta 2017, 20.10.05 EEST Thomas Guillem a écrit :
> On Mon, Jun 5, 2017, at 19:59, Rémi Denis-Courmont wrote:
> > ---
> > 
> >  include/vlc_picture.h        | 1 +
> >  modules/hw/vdpau/picture.c   | 4 ++--
> >  modules/hw/vdpau/vlc_vdpau.h | 3 +--
> 
> hw/vdpau changes should be in a separated patch.

Hell no. That would break incremental builds.

> >  src/misc/picture.c           | 5 +++++
> >  4 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/vlc_picture.h b/include/vlc_picture.h
> > index a48a053ac6..e25374634f 100644
> > --- a/include/vlc_picture.h
> > +++ b/include/vlc_picture.h
> > @@ -59,6 +59,7 @@ typedef struct plane_t
> > 
> >  typedef struct picture_context_t
> >  {
> >  
> >      void (*destroy)(struct picture_context_t *);
> > 
> > +    struct picture_context_t *(*copy)(struct picture_context_t *);
> > 
> >  } picture_context_t;
> >  
> >  /**
> > 
> > diff --git a/modules/hw/vdpau/picture.c b/modules/hw/vdpau/picture.c
> > index 88801167a5..4c42eef719 100644
> > --- a/modules/hw/vdpau/picture.c
> > +++ b/modules/hw/vdpau/picture.c
> > @@ -65,7 +65,7 @@ static picture_context_t *SurfaceCopy(picture_context_t
> > *ctx)
> > 
> >          return NULL;
> >      
> >      fnew->context.destroy = SurfaceDestroy;
> > 
> > -    fnew->copy = SurfaceCopy;
> > +    fnew->context.copy = SurfaceCopy;
> > 
> >      fnew->frame = frame;
> >      fnew->structure = fold->structure;
> >      fnew->procamp = fold->procamp;
> > 
> > @@ -98,7 +98,7 @@ vlc_vdp_video_field_t *vlc_vdp_video_create(vdp_t *vdp,
> > 
> >      }
> >      
> >      field->context.destroy = SurfaceDestroy;
> > 
> > -    field->copy = SurfaceCopy;
> > +    field->context.copy = SurfaceCopy;
> > 
> >      field->frame = frame;
> >      field->structure = VDP_VIDEO_MIXER_PICTURE_STRUCTURE_FRAME;
> >      field->procamp = procamp_default;
> > 
> > diff --git a/modules/hw/vdpau/vlc_vdpau.h b/modules/hw/vdpau/vlc_vdpau.h
> > index 318425c79b..af62bddcc1 100644
> > --- a/modules/hw/vdpau/vlc_vdpau.h
> > +++ b/modules/hw/vdpau/vlc_vdpau.h
> > @@ -274,7 +274,6 @@ typedef struct vlc_vdp_video_frame
> > 
> >  typedef struct vlc_vdp_video_field
> >  {
> >  
> >      picture_context_t context;
> > 
> > -    struct picture_context_t *(*copy)(struct picture_context_t *);
> > 
> >      vlc_vdp_video_frame_t *frame;
> >      VdpVideoMixerPictureStructure structure;
> >      VdpProcamp procamp;
> > 
> > @@ -303,6 +302,6 @@ static inline void
> > vlc_vdp_video_destroy(vlc_vdp_video_field_t *f)
> > 
> >  static inline vlc_vdp_video_field_t *vlc_vdp_video_copy(
> >  
> >      vlc_vdp_video_field_t *fold)
> >  
> >  {
> > 
> > -    return (vlc_vdp_video_field_t *)fold->copy(&fold->context);
> > +    return (vlc_vdp_video_field_t *)fold->context.copy(&fold->context);
> > 
> >  }
> >  #endif
> > 
> > diff --git a/src/misc/picture.c b/src/misc/picture.c
> > index 3f37648985..0cb251255b 100644
> > --- a/src/misc/picture.c
> > +++ b/src/misc/picture.c
> > @@ -368,6 +368,11 @@ void picture_CopyPixels( picture_t *p_dst, const
> > picture_t *p_src )
> > 
> >  {
> >  
> >      for( int i = 0; i < p_src->i_planes ; i++ )
> >      
> >          plane_CopyPixels( p_dst->p+i, p_src->p+i );
> > 
> > +
> > +    assert( p_dst->context == NULL );
> > +
> > +    if( p_src->context != NULL )
> > +        p_dst->context = p_src->context->copy( p_src->context );
> > 
> >  }
> 
> This looks good, but can you wait a little in order to make sure that we
> can implement it on CVPX/VAAPI/D3D*

And then what?
Duplicate the existing VDPAU code in the core? Not acceptable.
Drop support for VDPAU? I know that VDPAU is essentially dead. But VLC will 
not switch to CUDA in version 3.0 time frame, and there are no alternatives 
for NVIDIA on Linux, since VA is not supported natively.

This callback prototype can handle deep copy, shallow copy and reference 
counting indistinctly. It can also fail safe if copy is impossible (return 
NULL), whether systematically or accidentally. As already noted, it does not 
add any extra constraints for the back-end that would not come from picture 
context usage. In fact, it is very much a fix for the originally incomplete 
implementation of picture context.

The snapshot code requires a filter to copy to CPU. That is definitely not 
universally supported. But this patch has little to do with it.


-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/