libVLC 3.0.0 vmem lock/unlock order

This forum is about all development around libVLC.
lnbb
New Cone
New Cone
Posts: 2
Joined: 13 Sep 2016 09:45

libVLC 3.0.0 vmem lock/unlock order

Postby lnbb » 13 Sep 2016 09:57

Hi,

it seems that something recently changed the locking in the vmem video output, as the SDL 2.0 example code does not work anymore.

I thought the callbacks given to video_set_callbacks are called this way:

Code: Select all

lock() unlock() display() lock() unlock() display() ...
Instead using the recent git version they are called this way:

Code: Select all

lock() display() display() display() ... display() unlock()
Is that a bug? How should one now read from the pixels buffer if it is always "locked"? There is no way to find out if vlc is done writing to the buffer.

The documentation still says "A picture buffer is unlocked after the picture is decoded, but before the picture is displayed.", that seems to be wrong now.

Rémi Denis-Courmont
Developer
Developer
Posts: 15268
Joined: 07 Jun 2004 16:01
VLC version: master
Operating System: Linux
Contact:

Re: libVLC 3.0.0 vmem lock/unlock order

Postby Rémi Denis-Courmont » 13 Sep 2016 10:01

Known bug
Rémi Denis-Courmont
https://www.remlab.net/
Private messages soliciting support will be systematically discarded

lnbb
New Cone
New Cone
Posts: 2
Joined: 13 Sep 2016 09:45

Re: libVLC 3.0.0 vmem lock/unlock order

Postby lnbb » 13 Sep 2016 10:57

Thanks for the info. I just did a quick search and found the following bug entry, that seems to track the issue: https://trac.videolan.org/vlc/ticket/17187

oviano
Cone that earned his stripes
Cone that earned his stripes
Posts: 120
Joined: 12 Jan 2012 11:12

Re: libVLC 3.0.0 vmem lock/unlock order

Postby oviano » 25 Oct 2016 14:04

Unfortunately the fix for this bug made on 15th September quite significantly increases the CPU usage of my application because it's now doing an extra copy of the buffer every frame.

In my case, I don't need it. It's an SDL app that works like this:

- since it's SDL, all texture/render calls have to be in the main thread (the SDL sample code on the VLC wiki will eventually crash because it breaks this rule, I've verified this)
- libVLC decodes to the memory buffer I give it in the setup callback
- in the display callback I signal my main/render thread which then copies the memory buffer into a texture. The display callback then continues. The texture is then rendered and the UI texture onto.

So the additional copy that this patch introduced is completely unnecessary in my use case and simply increases the CPU.

I've obviously built a local VLC lib that reverts this change, but maybe this behaviour can be configurable?

I understand that the fix makes some (maybe most?) use cases safer but an inefficient system is now even more inefficient! I know there is a comment in the patch discussion which mentions VLC 4.0+ and writing to an OpenGL frame buffer. That's great news but let's face it isn't going to be anytime soon as 3.0 isn't even finished yet.

So maybe in the meantime, it could be possible for the libVLC client API user to tell the vmem output system that we're only going to be touching the buffer in the display callback? Maybe set a flag or something in the setup callback to control this behaviour?

Rémi Denis-Courmont
Developer
Developer
Posts: 15268
Joined: 07 Jun 2004 16:01
VLC version: master
Operating System: Linux
Contact:

Re: libVLC 3.0.0 vmem lock/unlock order

Postby Rémi Denis-Courmont » 25 Oct 2016 14:20

For performance, render in a OS window handle. vmem will never be efficient, and there are no ways with the current API to safely improve it.

And frankly, it is basically impossible for an application to not need the last fixes, unless you have oracle.
Rémi Denis-Courmont
https://www.remlab.net/
Private messages soliciting support will be systematically discarded

oviano
Cone that earned his stripes
Cone that earned his stripes
Posts: 120
Joined: 12 Jan 2012 11:12

Re: libVLC 3.0.0 vmem lock/unlock order

Postby oviano » 25 Oct 2016 16:28

Thanks Rémi.

So are you saying that (going on the pre-patch code) that some thread inside the engine somewhere could still be writing to the picture buffer at the same time as the Display function is called? Because during the display callback is the only time that I am accessing the buffer, apart from during setup when it is allocated and cleanup when it is freed.

Regarding using an OS window - this doesn't seem compatible with doing things like overlaying SDL textures etc.

Rémi Denis-Courmont
Developer
Developer
Posts: 15268
Joined: 07 Jun 2004 16:01
VLC version: master
Operating System: Linux
Contact:

Re: libVLC 3.0.0 vmem lock/unlock order

Postby Rémi Denis-Courmont » 25 Oct 2016 17:21

Without the latest patch, the API does not convey enough informations to the application to interpret the buffers correctly.

You can of course try to second guess the missing parameters, but that will eventually lead to failure.
Rémi Denis-Courmont
https://www.remlab.net/
Private messages soliciting support will be systematically discarded

oviano
Cone that earned his stripes
Cone that earned his stripes
Posts: 120
Joined: 12 Jan 2012 11:12

Re: libVLC 3.0.0 vmem lock/unlock order

Postby oviano » 25 Oct 2016 19:20

Ok, so it fixes other stuff aswell, cool.

So I've been looking at the code (latest wth your patch of 15th September). My understanding is that:

1) libvlc allocates some planes for the decoding
2) in Prepare, it calls lock() to get my buffers, copies the planes into them, and then calls unlock()
3) in Display, it basically expects me to draw my buffer to the screen

However, I noticed that Display also receives the internal "pic" pointer containing the actual buffer that the engine is decoding to, but of course this isn't passed onto the display callback.

So my question is could that theoretically be used in the display_cb?

I was thinking of something like this:

- allow lock_cb to be NULL
- if lock_cb is NULL then basically do nothing in Prepare at all
- then, in Display if the lock_cb wasn't supplied, then instead pass the planes (from pic) down to display_cb

In my scenario this would be ideal because I'd then access the planes to update my texture and then return from the display_cb.

Is that safe? Presumably it must be because otherwise why would Display be being sent a "pic" pointer in the first place if it couldn't use it?

When operating in this mode it would also simplify my code. I would be able to ditch the lock_cb and unlock_cb functions entirely, and I'd no longer even need to allocate any memory because I would be taking it directly from the engine.

Rémi Denis-Courmont
Developer
Developer
Posts: 15268
Joined: 07 Jun 2004 16:01
VLC version: master
Operating System: Linux
Contact:

Re: libVLC 3.0.0 vmem lock/unlock order

Postby Rémi Denis-Courmont » 25 Oct 2016 22:00

I don't see the point. Prepare is called "as early as possible", whereas "Display" is called more-or-less exactly when the picture is expected to be rendered. Updating the texture in Display seems counter-productive.

With that being said, if you want efficient rendering into something that is not an OS window, you should write your own video output plugin.
Rémi Denis-Courmont
https://www.remlab.net/
Private messages soliciting support will be systematically discarded

oviano
Cone that earned his stripes
Cone that earned his stripes
Posts: 120
Joined: 12 Jan 2012 11:12

Re: libVLC 3.0.0 vmem lock/unlock order

Postby oviano » 25 Oct 2016 22:23

The point would be to eliminate the pixel copy in Prepare.

Point taken re: the best place to update the texture being in Prepare rather than Display though...so probably in that case I would be best updating the texture in the unlock, by modifying things so that the internal planes are passed to unlock. Then display just triggers the texture to be rendered to the screen.

In terms of writing a plugin - yeah that would be optimum, but the thing is I get good enough performance with 1080p HEVC decoding using libVLC in this way on a pretty moderate spec PC (Haswell NUC from a few years ago). Eliminating the copy, as per the pre-patch code, is definitely using less CPU than latest code.

Thankyou for taking the time to feedback on my ideas and helping me to understand this bit of the code, anyway :)

oviano
Cone that earned his stripes
Cone that earned his stripes
Posts: 120
Joined: 12 Jan 2012 11:12

Re: libVLC 3.0.0 vmem lock/unlock order

Postby oviano » 27 Oct 2016 08:33

So here is a patch that I'm using for my build of libVLC. Now, if you leave the unlock callback as NULL it takes this as meaning you're not going to be supplying a buffer and the internal buffer should be passed directly to the display callback.

Essentially, before:

- i allocate a buffer in setup callback
- between lock and unlock i don't touch it, because libVLC is copying from its buffer into it
- in display i copy it to the texture and render
- in cleanup i free the buffer

After:

- in display i copy the internal buffer to the texture and render
- no need for a lock, unlock or cleanup function. Setup just makes any changes to chroma, size etc that i require.

From your comments above you probably don't agree with this change to the API and that's totally fine, but I'm sharing it just in case. Or maybe someone else would find it useful. One thing I couldn't figure out was what to change to enable "npapi" to build on Windows when you run make package-win32-zip. At the moment with this patch it fails due to the change in the display callback signature.

Code: Select all

From bd649e5ba8040b9da8eb9f3ac445a52d994aabac Mon Sep 17 00:00:00 2001 From: Oliver Collyer <ovcollyer@mac.com> Date: Wed, 26 Oct 2016 07:39:38 +0300 Subject: [PATCH 1/1] Optionally pass decode buffer to display cb callback instead of copying --- include/vlc/libvlc_media_player.h | 3 ++- lib/media_player.c | 2 +- modules/video_output/vmem.c | 29 ++++++++++++++++++++++------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/include/vlc/libvlc_media_player.h b/include/vlc/libvlc_media_player.h index 9a73b4f..f469e98 100644 --- a/include/vlc/libvlc_media_player.h +++ b/include/vlc/libvlc_media_player.h @@ -323,7 +323,8 @@ typedef void (*libvlc_video_unlock_cb)(void *opaque, void *picture, * \param picture private pointer returned from the @ref libvlc_video_lock_cb * callback [IN] */ -typedef void (*libvlc_video_display_cb)(void *opaque, void *picture); +typedef void (*libvlc_video_display_cb)(void *opaque, void *picture, + void *const *planes); /** * Callback prototype to configure picture buffers format. diff --git a/lib/media_player.c b/lib/media_player.c index db08274..29fa43c 100644 --- a/lib/media_player.c +++ b/lib/media_player.c @@ -1073,7 +1073,7 @@ int libvlc_media_player_set_renderer( libvlc_media_player_t *p_mi, void libvlc_video_set_callbacks( libvlc_media_player_t *mp, void *(*lock_cb) (void *, void **), void (*unlock_cb) (void *, void *, void *const *), - void (*display_cb) (void *, void *), + void (*display_cb) (void *, void *, void *const *), void *opaque ) { var_SetAddress( mp, "vmem-lock", lock_cb ); diff --git a/modules/video_output/vmem.c b/modules/video_output/vmem.c index 25d8a86..1c82ec8 100644 --- a/modules/video_output/vmem.c +++ b/modules/video_output/vmem.c @@ -93,7 +93,7 @@ struct vout_display_sys_t { void *pic_opaque; void *(*lock)(void *sys, void **plane); void (*unlock)(void *sys, void *id, void *const *plane); - void (*display)(void *sys, void *id); + void (*display)(void *sys, void *id, void *const *plane); void (*cleanup)(void *sys); unsigned pitches[PICTURE_PLANE_MAX]; @@ -124,11 +124,6 @@ static int Open(vlc_object_t *object) vlc_format_cb setup = var_InheritAddress(vd, "vmem-setup"); sys->lock = var_InheritAddress(vd, "vmem-lock"); - if (sys->lock == NULL) { - msg_Err(vd, "missing lock callback"); - free(sys); - return VLC_EGENERIC; - } sys->unlock = var_InheritAddress(vd, "vmem-unlock"); sys->display = var_InheritAddress(vd, "vmem-display"); sys->cleanup = var_InheritAddress(vd, "vmem-cleanup"); @@ -255,6 +250,11 @@ static void Prepare(vout_display_t *vd, picture_t *pic, subpicture_t *subpic) picture_resource_t rsc = { .p_sys = NULL }; void *planes[PICTURE_PLANE_MAX]; + if (sys->lock == NULL) { + sys->pic_opaque = NULL; + return; + } + sys->pic_opaque = sys->lock(sys->opaque, planes); for (unsigned i = 0; i < PICTURE_PLANE_MAX; i++) { @@ -279,8 +279,23 @@ static void Display(vout_display_t *vd, picture_t *pic, subpicture_t *subpic) { vout_display_sys_t *sys = vd->sys; + void *planes[PICTURE_PLANE_MAX]; + if (sys->lock == NULL) { + for (unsigned i = 0; i < PICTURE_PLANE_MAX; i++) { + if (i < (unsigned)pic->i_planes) { + plane_t *p_src = pic->p+i; + planes[i] = p_src->p_pixels; + } else + planes[i] = NULL; + } + } else { + for (unsigned i = 0; i < PICTURE_PLANE_MAX; i++) { + planes[i] = NULL; + } + } + if (sys->display != NULL) - sys->display(sys->opaque, sys->pic_opaque); + sys->display(sys->opaque, sys->pic_opaque, planes); picture_Release(pic); VLC_UNUSED(subpic); -- 2.7.4

Jean-Baptiste Kempf
Site Administrator
Site Administrator
Posts: 37523
Joined: 22 Jul 2005 15:29
VLC version: 4.0.0-git
Operating System: Linux, Windows, Mac
Location: Cone, France
Contact:

Re: libVLC 3.0.0 vmem lock/unlock order

Postby Jean-Baptiste Kempf » 07 Dec 2016 00:36

This is an interesting hack, indeed.
Jean-Baptiste Kempf
http://www.jbkempf.com/ - http://www.jbkempf.com/blog/category/Videolan
VLC media player developer, VideoLAN President and Sites administrator
If you want an answer to your question, just be specific and precise. Don't use Private Messages.


Return to “Development around libVLC”

Who is online

Users browsing this forum: No registered users and 8 guests