[FFmpeg-devel] [PATCH] lavd/sdl2: postpone sdl2 window creation

[Andreas Rheinhardt]

Xiang, Haihao:
> From: Haihao Xiang <haihao.xiang at intel.com>
> 
> Since 2d924b3, sdl2_write_header() and sdl2_write_packet() are called
> in two different threads. However SDL2 requires window creation and
> rendering should be done in the same thread, otherwise it shows nothing
> when specifying SDL2 output device.

Modifying a library to fix behaviour in an application (even the FFmpeg
tool) is very fishy. This patch makes things worse for people who want
their call to avformat_write_header() to actually check whether sdl2 works.

> 
> $ ffmpeg -f lavfi -i yuvtestsrc -pix_fmt yuv420p -f sdl2 "sdl2"
> 
> Signed-off-by: Haihao Xiang <haihao.xiang at intel.com>
> ---
>  libavdevice/sdl2.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/libavdevice/sdl2.c b/libavdevice/sdl2.c
> index 342a253dc0..c9f7f03c28 100644
> --- a/libavdevice/sdl2.c
> +++ b/libavdevice/sdl2.c
> @@ -158,6 +158,11 @@ static int sdl2_write_trailer(AVFormatContext *s)
>  }
>  
>  static int sdl2_write_header(AVFormatContext *s)
> +{
> +    return 0;
> +}

There is no reason to keep an empty write-header function around. Just
delete the function pointer in the FFOutputFormat.

> +
> +static int sdl2_init(AVFormatContext *s)
>  {
>      SDLContext *sdl = s->priv_data;
>      AVStream *st = s->streams[0];
> @@ -165,6 +170,9 @@ static int sdl2_write_header(AVFormatContext *s)
>      int i, ret = 0;
>      int flags  = 0;
>  
> +    if (sdl->inited)

I am not an sdl2-expert at all (in fact, your patch made me read their
headers for the first time), but it seems to me that you misread the
semantics of "inited": It is set if someone else already initialized the
library or if we called sdl2_write_header() without entering the fail
path. In particular, inited being set does not mean that we called
sdl2_write_header() without entering the fail path.

The reason I am writing "without entering the fail path" and not
"returns an error" is that sdl2_write_header() actually never sets an
error on any error path. This means that the sdl2_init() below will
always succeed.

Given that inited is currently only used for one thing (namely checking
whether SDL_Quit() should be called), it would be equivalent to the
current code to move the call to SDL_Quit() to the error path in
sdl2_write_header() and to make inited a local variable in
sdl2_write_header().

The reason for the way inited is handled seems to me that because
SDL_Quit() quits everything (and discards reference counters etc.), it
may only be called if no one else uses SDL2, hence not calling
SDL_Quit() if it was already initialized or if someone else may have
initialized it after we have have returned via the non-error path in
sdl2_write_header().

I think that this is wrong even if all interactions with SDL2 happen
only in one thread: The check for whether SDL2 was already initialized
only checks for whether the video subsystem was initialized. But other
subsystems might have already been initialized and these would also be
killed by SDL_Quit().

Therefore I think that we should never call SDL_Quit(). Instead we
should call SDL_InitSubSystem() and call SDL_QuitSubSystem() iff
SDL_InitSubSystem() succeeded. This is refcounted. And then
write_trailer should be made a deinit function.

I just made a quick test and this reduces the still reachable amount of
memory at the end (as reported by Valgrind) considerably (from 5,314,497
to 312,104).

Sorry for this rant which does not affect your threading issue at all.

> +        return 0;
> +
>      if (!sdl->window_title)
>          sdl->window_title = av_strdup(s->url);
>  
> @@ -249,6 +257,11 @@ static int sdl2_write_packet(AVFormatContext *s, AVPacket *pkt)
>      int linesize[4];
>  
>      SDL_Event event;
> +
> +    ret = sdl2_init(s);
> +    if (ret)
> +        return ret;
> +
>      if (SDL_PollEvent(&event)){
>          switch (event.type) {
>          case SDL_KEYDOWN: