[vlc-devel] [vlc-commits] vout: fix picture lock/unlock with private pool

[Thomas Guillem]

I had some time to think and look into the code, I may have a plan for a
better android vout:

Merge android/opaque.c and android/surface.c into one vout, Keep old
part of surface.c in an other bad vout for old devices. That way, even
with software rendering, we can render subtitles on a new android
surface like opaque does (and not have 2 way to render subpictures).

The new vout will use a normal picture Pool without lock/unlock for
non-dr.
But, I can't find a way to not use pool->pic_unlock when using direct
rendering: we need a way to release the picture on mediacodec/omx side
if it's released and not displayed.

Pictures allocated by the pool will have a p_sys with some callbacks:

 - pf_get_pic:
     - androidLockSurface (default)
     - NULL (set by mediacodec/omx dr)

 - pf_copy_pic:
     - memcpy (default).
     - OMXCopy (set by mediacodec/omx non-dr)
     - NULL (set by mediacodec/omx dr)

 - pf_display_pic:
     - androidUnlockSurface(default)
     - private mediacodec/omx callback (dr).

 - pf_release_pic:
     - private mediacodec/omx callback (dr).

Ideally, pictures allocated by the pool have pixels allocated by vlc
only for non-dr, non omx, non mediacodec (so, for avcodec).

Then, to prepare, display pictures:
 - vd->prepare(): call pf_get_pick, then pf_copy_pic if not NULL.
 - vd->display(): call pf_display_pic (and render subpictures).
 - vd->pool()->pic_unlock: call pf_release_pic if picture not displayed.

What do you think ?


On Tue, Nov 4, 2014, at 09:53, Thomas Guillem wrote:
> 
> 
> On Mon, Nov 3, 2014, at 21:19, Rémi Denis-Courmont wrote:
> > Le lundi 03 novembre 2014, 15:35:37 Thomas Guillem a écrit :
> > > With Mediacodec, we use android/opaque vout that use a pool with lock /
> > > unlock. Because of that, we don't use the code patch where direct
> > > rendering is enabled.
> > 
> > Combining DR and picture locking does not make much sense. With DR, the 
> > picture may still be referenced by the decoder or filters while it gets 
> > displayed. So I don't see the point in (locking and) unlocking then...
> > 
> > You might just as well call the locking code from the pool() callback and
> > the 
> > unlocking code at plugin deactivation.
> > 
> > Then again, I don't think locking makes much sense at all. Even without
> > DR, 
> > prepare() seems more practical than unlock() and picture_Release() than 
> > lock().
> 
> I agree.
> 
> > 
> > > A solution for my problem is to move the "!picture_pool_NeedsLocking()"
> > > check and have, at the end of vout_InitWrapper:
> > > 
> > > if (!picture_pool_NeedsLocking(sys->decoder_pool))
> > >     sys->private_pool = picture_pool_Reserve(sys->decoder_pool,
> > >     private_picture);
> > > 
> > > I have a working patch, but I'm not sure about all the consequences.
> > 
> > vout->p->private_pool will be NULL and the process will crash if the 
> > private_pool is ever required (see src/video_output/video_output.c).
> > 
> > Assuming the video output supports direct rendering and subpicture
> > blending, 
> > the private pool seems to be needed for:
> > - snapshot while a subpicture is present,
> > - filter-allocated pictures.
> > And there are some more cases otherwise. Maybe none of those ever happens
> > on 
> > Android but in general, they most definitely do.
> 
> I proposed 2 patches to fix mediacodec on master with current pool
> lock/unlock.
> 
> Even though, I'll try to have a look at both android surface and opaque
> vout to get rid of picture looking.
> 
> > 
> > -- 
> > Rémi Denis-Courmont
> > http://www.remlab.net/
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel