Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-18 Thread Xiaolei Yu 
On 01/18/2016 03:52 AM, Mark Thompson wrote:
> On 17/01/16 19:46, Mark Thompson wrote:
>> On 17/01/16 18:46, wm4 wrote:
>>>
>>> There are two issues:
>>> 1. global state in libav* which is not synchronized
>>> 2. thread-safety within
>>>
>>> 1. is is completely unacceptable, because it can trigger undefined
>>> behavior if there is more than 1 libav* user in the same process. I'm
>>> not really convinced that a "device string" is really reliably unique
>>> enough that it won't be a problem across library users. (For example,
>>> it's entirely possible enough to open 2 X11 Displays to the same X
>>> server using the same display name.)
>>
>> Ok, I'm happy with the first part of that (and that it is fixable by a
>> simple lock around the connection initialisation, assuming this code
>> stays in libavutil).
>>
>> Can you offer an example where the device strings actually create a
>> problem?
>>
>> Multiple users within the same process /must/ be given the same
>> connection if they ask for the same device, because we have no way to
>> distinguish different sets of instances which want to be able to work
>> together.  Equally, two connections to the same device under different
>> names are acceptably different, because they won't have come from the
>> same instance set.
> 
> Right, I see the problem.  The user will want to do something with the 
> surface they get back under the same X11 display handle.  We can't call 
> XOpenDisplay() in that case: the user has to be able to pass their own handle 
> in.  So we need some other way to register that connection.
> 
>>
>>> With 2. it's a bit more complicated. There should probably indeed be
>>> something like a big lock around all uses of the same VADisplay, as
>>> long as libva exhibits this problem.
>>
>> This is straightforward to do, if tedious.
>>
>> Can you explain the ABI and API constraints on changes to existing
>> structures?
>>
>> For the existing decoders (and their users) to work, it will require
>> either:
>> (a) a global list of connections somewhere to map VADisplay to lock
>> or
>> (b) an additional member in struct vaapi_context to point to the lock.
>>
>> If ABI and API compatibility is required for existing users then (b) is
>> out, and we have to have the global list (suitably locked).
>>
>> If we can break both then the right answer is probably to pass
>> hwaccel_context to encoders as well, and add a similar field to
>> AVFilterContext to use there too.
>>
>> If ABI compatibility is required but an API break is allowed then we
>> could do horrible things to hack (b) into working.  For example, replace
>> the VADisplay pointer in the first member of struct vaapi_context to
>> instead point at a new structure which contains some magic bytes at the
>> start.  If the magic bytes are where that pointer goes then we are using
>> the new API and can lock using that, and if they are not found then it
>> was a user-provided VADisplay and no locking is required.
>>
>> - Mark
>>
>>
>> PS:  I have no attachment to this piece of code (around connection
>> initialisation) at all; it was just required to make everything else
>> work.  If you want to suggest a better and completely different approach
>> then I am happy to throw it all away and start again.
>>

I think you can supply VADisplay to AVCodecContext through av_opt_ptr and
leave its initialization to user.

> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-17 Thread Mark Thompson 

On 17/01/16 17:53, wm4 wrote:

On Sun, 17 Jan 2016 17:34:55 +
Mark Thompson  wrote:


  From 2442c1aca8778167c2e60a34d03ed452737f1366 Mon Sep 17 00:00:00 2001
From: Mark Thompson 
Date: Sun, 17 Jan 2016 15:48:54 +
Subject: [PATCH 1/5] libavutil: Some VAAPI infrastructure




+
+static AVVAAPIConnection *av_vaapi_connection_list;
+
+int av_vaapi_instance_init(AVVAAPIInstance *instance, const char *device)
+{
+AVVAAPIConnection *ctx;
+int err;
+
+for(ctx = av_vaapi_connection_list; ctx; ctx = ctx->next) {
+if((device == 0 && ctx->device_string == 0) ||
+   (device && ctx->device_string &&
+!strcmp(device, ctx->device_string)))
+break;
+}


This won't work. Neither vaapi nor your patch are thread-safe, yet you
have them as very central global mutable state.



True.  That setup is all pretty nasty, and everything currently assumes 
it happens on the same thread.  Since multiple instances have to use a 
common connection to libva (because they have to be able to pass 
surfaces between them), this is unfortunately pretty much required.


If multithreaded use is desirable immediately then we could just have a 
big lock which anything VAAPI-related must take when it wants to do 
anything?  (This would require changes to all existing VAAPI decoders as 
well.)


- Mark

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-17 Thread Hendrik Leppkes 
On Sun, Jan 17, 2016 at 7:13 PM, Mark Thompson  wrote:
> On 17/01/16 17:53, wm4 wrote:
>>
>> On Sun, 17 Jan 2016 17:34:55 +
>> Mark Thompson  wrote:
>>
>>>   From 2442c1aca8778167c2e60a34d03ed452737f1366 Mon Sep 17 00:00:00 2001
>>> From: Mark Thompson 
>>> Date: Sun, 17 Jan 2016 15:48:54 +
>>> Subject: [PATCH 1/5] libavutil: Some VAAPI infrastructure
>>>
>>
>>> +
>>> +static AVVAAPIConnection *av_vaapi_connection_list;
>>> +
>>> +int av_vaapi_instance_init(AVVAAPIInstance *instance, const char
>>> *device)
>>> +{
>>> +AVVAAPIConnection *ctx;
>>> +int err;
>>> +
>>> +for(ctx = av_vaapi_connection_list; ctx; ctx = ctx->next) {
>>> +if((device == 0 && ctx->device_string == 0) ||
>>> +   (device && ctx->device_string &&
>>> +!strcmp(device, ctx->device_string)))
>>> +break;
>>> +}
>>
>>
>> This won't work. Neither vaapi nor your patch are thread-safe, yet you
>> have them as very central global mutable state.
>>
>
> True.  That setup is all pretty nasty, and everything currently assumes it
> happens on the same thread.  Since multiple instances have to use a common
> connection to libva (because they have to be able to pass surfaces between
> them), this is unfortunately pretty much required.
>
> If multithreaded use is desirable immediately then we could just have a big
> lock which anything VAAPI-related must take when it wants to do anything?
> (This would require changes to all existing VAAPI decoders as well.)
>

static variables (ie. global state) are undesirable as a concept entirely.
Applications that want to setup a chain with pass through should
manage the needed connection and make it available to each component
needing access to it.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-17 Thread wm4 
On Sun, 17 Jan 2016 18:13:50 +
Mark Thompson  wrote:

> On 17/01/16 17:53, wm4 wrote:
> > On Sun, 17 Jan 2016 17:34:55 +
> > Mark Thompson  wrote:
> >  
> >>   From 2442c1aca8778167c2e60a34d03ed452737f1366 Mon Sep 17 00:00:00 2001
> >> From: Mark Thompson 
> >> Date: Sun, 17 Jan 2016 15:48:54 +
> >> Subject: [PATCH 1/5] libavutil: Some VAAPI infrastructure
> >>  
> >  
> >> +
> >> +static AVVAAPIConnection *av_vaapi_connection_list;
> >> +
> >> +int av_vaapi_instance_init(AVVAAPIInstance *instance, const char *device)
> >> +{
> >> +AVVAAPIConnection *ctx;
> >> +int err;
> >> +
> >> +for(ctx = av_vaapi_connection_list; ctx; ctx = ctx->next) {
> >> +if((device == 0 && ctx->device_string == 0) ||
> >> +   (device && ctx->device_string &&
> >> +!strcmp(device, ctx->device_string)))
> >> +break;
> >> +}  
> >
> > This won't work. Neither vaapi nor your patch are thread-safe, yet you
> > have them as very central global mutable state.
> >  
> 
> True.  That setup is all pretty nasty, and everything currently assumes 
> it happens on the same thread.  Since multiple instances have to use a 
> common connection to libva (because they have to be able to pass 
> surfaces between them), this is unfortunately pretty much required.
> 
> If multithreaded use is desirable immediately then we could just have a 
> big lock which anything VAAPI-related must take when it wants to do 
> anything?  (This would require changes to all existing VAAPI decoders as 
> well.)

There are two issues:
1. global state in libav* which is not synchronized
2. thread-safety within 

1. is is completely unacceptable, because it can trigger undefined
behavior if there is more than 1 libav* user in the same process. I'm
not really convinced that a "device string" is really reliably unique
enough that it won't be a problem across library users. (For example,
it's entirely possible enough to open 2 X11 Displays to the same X
server using the same display name.)

With 2. it's a bit more complicated. There should probably indeed be
something like a big lock around all uses of the same VADisplay, as
long as libva exhibits this problem.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-17 Thread Mark Thompson 

On 17/01/16 19:46, Mark Thompson wrote:

On 17/01/16 18:46, wm4 wrote:


There are two issues:
1. global state in libav* which is not synchronized
2. thread-safety within

1. is is completely unacceptable, because it can trigger undefined
behavior if there is more than 1 libav* user in the same process. I'm
not really convinced that a "device string" is really reliably unique
enough that it won't be a problem across library users. (For example,
it's entirely possible enough to open 2 X11 Displays to the same X
server using the same display name.)


Ok, I'm happy with the first part of that (and that it is fixable by a
simple lock around the connection initialisation, assuming this code
stays in libavutil).

Can you offer an example where the device strings actually create a
problem?

Multiple users within the same process /must/ be given the same
connection if they ask for the same device, because we have no way to
distinguish different sets of instances which want to be able to work
together.  Equally, two connections to the same device under different
names are acceptably different, because they won't have come from the
same instance set.


Right, I see the problem.  The user will want to do something with the 
surface they get back under the same X11 display handle.  We can't call 
XOpenDisplay() in that case: the user has to be able to pass their own 
handle in.  So we need some other way to register that connection.





With 2. it's a bit more complicated. There should probably indeed be
something like a big lock around all uses of the same VADisplay, as
long as libva exhibits this problem.


This is straightforward to do, if tedious.

Can you explain the ABI and API constraints on changes to existing
structures?

For the existing decoders (and their users) to work, it will require
either:
(a) a global list of connections somewhere to map VADisplay to lock
or
(b) an additional member in struct vaapi_context to point to the lock.

If ABI and API compatibility is required for existing users then (b) is
out, and we have to have the global list (suitably locked).

If we can break both then the right answer is probably to pass
hwaccel_context to encoders as well, and add a similar field to
AVFilterContext to use there too.

If ABI compatibility is required but an API break is allowed then we
could do horrible things to hack (b) into working.  For example, replace
the VADisplay pointer in the first member of struct vaapi_context to
instead point at a new structure which contains some magic bytes at the
start.  If the magic bytes are where that pointer goes then we are using
the new API and can lock using that, and if they are not found then it
was a user-provided VADisplay and no locking is required.

- Mark


PS:  I have no attachment to this piece of code (around connection
initialisation) at all; it was just required to make everything else
work.  If you want to suggest a better and completely different approach
then I am happy to throw it all away and start again.



___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-17 Thread Mark Thompson 

From 2442c1aca8778167c2e60a34d03ed452737f1366 Mon Sep 17 00:00:00 2001
From: Mark Thompson 
Date: Sun, 17 Jan 2016 15:48:54 +
Subject: [PATCH 1/5] libavutil: Some VAAPI infrastructure

---
 configure  |   4 +
 libavutil/Makefile |   1 +
 libavutil/vaapi.c  | 732 
+

 libavutil/vaapi.h  | 116 +
 4 files changed, 853 insertions(+)
 create mode 100644 libavutil/vaapi.c
 create mode 100644 libavutil/vaapi.h

diff --git a/configure b/configure
index 7cef6f5..1c77015 100755
--- a/configure
+++ b/configure
@@ -5739,6 +5739,10 @@ enabled vaapi && enabled xlib &&
 check_lib2 "va/va.h va/va_x11.h" vaGetDisplay -lva -lva-x11 &&
 enable vaapi_x11

+enabled vaapi &&
+check_lib2 "va/va.h va/va_drm.h" vaGetDisplayDRM -lva -lva-drm &&
+enable vaapi_drm
+
 enabled vdpau &&
 check_cpp_condition vdpau/vdpau.h "defined 
VDP_DECODER_PROFILE_MPEG4_PART2_ASP" ||

 disable vdpau
diff --git a/libavutil/Makefile b/libavutil/Makefile
index bf8c713..8025f9f 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -146,6 +146,7 @@ OBJS-$(!HAVE_ATOMICS_NATIVE)+= atomic.o 
\


 OBJS-$(CONFIG_LZO)  += lzo.o
 OBJS-$(CONFIG_OPENCL)   += opencl.o opencl_internal.o
+OBJS-$(CONFIG_VAAPI)+= vaapi.o

 OBJS += $(COMPAT_OBJS:%=../compat/%)

diff --git a/libavutil/vaapi.c b/libavutil/vaapi.c
new file mode 100644
index 000..cf0c790
--- /dev/null
+++ b/libavutil/vaapi.c
@@ -0,0 +1,732 @@
+/*
+ * VAAPI helper functions.
+ *
+ * Copyright (C) 2016 Mark Thompson 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
02110-1301 USA

+ */
+
+#include 
+
+#include 
+#include 
+
+#include "vaapi.h"
+
+#include 
+#include 
+
+#include "avassert.h"
+#include "imgutils.h"
+#include "pixfmt.h"
+
+
+static const AVClass vaapi_connection_class = {
+.class_name = "VAAPI/connection",
+.item_name  = av_default_item_name,
+.version= LIBAVUTIL_VERSION_INT,
+};
+
+static const AVClass vaapi_pipeline_class = {
+.class_name = "VAAPI/pipeline",
+.item_name  = av_default_item_name,
+.version= LIBAVUTIL_VERSION_INT,
+};
+
+typedef struct AVVAAPIConnection {
+const AVClass *class;
+
+char *device_string;
+int refcount;
+struct AVVAAPIInstance *next;
+
+VADisplay display;
+int initialised;
+int version_major, version_minor;
+
+enum {
+AV_VAAPI_CONNECTION_NONE = 0,
+AV_VAAPI_CONNECTION_DRM,
+AV_VAAPI_CONNECTION_X11,
+/* ?
+AV_VAAPI_CONNECTION_GLX,
+AV_VAAPI_CONNECTION_WAYLAND,
+*/
+} connection_type;
+union {
+void *x11_display;
+int drm_fd;
+};
+} AVVAAPIConnection;
+
+static int av_vaapi_connection_uninit(AVVAAPIConnection *ctx)
+{
+if(ctx->initialised) {
+vaTerminate(ctx->display);
+ctx->display = 0;
+ctx->initialised = 0;
+}
+
+switch(ctx->connection_type) {
+
+case AV_VAAPI_CONNECTION_DRM:
+if(ctx->drm_fd >= 0) {
+close(ctx->drm_fd);
+ctx->drm_fd = -1;
+}
+break;
+
+case AV_VAAPI_CONNECTION_X11:
+if(ctx->x11_display) {
+XCloseDisplay(ctx->x11_display);
+ctx->x11_display = 0;
+}
+break;
+
+}
+
+return 0;
+}
+
+static int av_vaapi_connection_init(AVVAAPIConnection *ctx, const char 
*device)

+{
+VAStatus vas;
+int err;
+
+ctx->class = _connection_class;
+if(device)
+ctx->device_string = av_strdup(device);
+
+// If the device name is not provided at all, assume we are in X 
and can
+// connect to the display in DISPLAY.  If we do get a device name 
and it

+// begins with a type indicator, use that.  Otherwise, try to guess the
+// answer from the content of the name.
+if(!device) {
+ctx->connection_type = AV_VAAPI_CONNECTION_X11;
+} else if(!strncmp(device, "drm:", 4)) {
+ctx->connection_type = AV_VAAPI_CONNECTION_DRM;
+device += 4;
+} else if(!strncmp(device, "x11:", 4)) {
+ctx->connection_type = AV_VAAPI_CONNECTION_X11;
+device += 4;
+} else {
+if(strchr(device,

Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

2016-01-17 Thread wm4 
On Sun, 17 Jan 2016 17:34:55 +
Mark Thompson  wrote:

>  From 2442c1aca8778167c2e60a34d03ed452737f1366 Mon Sep 17 00:00:00 2001
> From: Mark Thompson 
> Date: Sun, 17 Jan 2016 15:48:54 +
> Subject: [PATCH 1/5] libavutil: Some VAAPI infrastructure
> 

> +
> +static AVVAAPIConnection *av_vaapi_connection_list;
> +
> +int av_vaapi_instance_init(AVVAAPIInstance *instance, const char *device)
> +{
> +AVVAAPIConnection *ctx;
> +int err;
> +
> +for(ctx = av_vaapi_connection_list; ctx; ctx = ctx->next) {
> +if((device == 0 && ctx->device_string == 0) ||
> +   (device && ctx->device_string &&
> +!strcmp(device, ctx->device_string)))
> +break;
> +}

This won't work. Neither vaapi nor your patch are thread-safe, yet you
have them as very central global mutable state.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel