Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure
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
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
Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure
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. 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
Re: [FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure
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
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
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
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