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

From: Haihao Xiang <haihao.xiang@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.

$ ffmpeg -f lavfi -i yuvtestsrc -pix_fmt yuv420p -f sdl2 "sdl2"

Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
---
 libavdevice/sdl2.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Xiang, Haihao:
> From: Haihao Xiang <haihao.xiang@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@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:

On Ma, 2023-09-18 at 14:00 +0200, Andreas Rheinhardt wrote:
> Xiang, Haihao:
> > From: Haihao Xiang <haihao.xiang@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. 

Seems calling sdl2_write_header() and sdl2_write_packet() in two different
threads are allowed. I think the change in commit 2d924b3 revealed a bug in
libavdevice, we should fix libavdevice. 

> This patch makes things worse for people who want
> their call to avformat_write_header() to actually check whether sdl2 works.

sdl2_write_header() always returns 0 (you also pointed it out below), so this
change doesn't have impact to user if user want to use avformat_write_header()
to check whether sdl2 works. 

> 
> > 
> > $ ffmpeg -f lavfi -i yuvtestsrc -pix_fmt yuv420p -f sdl2 "sdl2"
> > 
> > Signed-off-by: Haihao Xiang <haihao.xiang@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.

Thanks for pointing it out, I will delete it in the next version if you agree we
should fix the library. 

> 
> > +
> > +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.

Honestly I am not an sdl2 expert too. I noticed sdl2_write_header() always
return 0, and sdl2_write_packet() is called even if sdl2_write_header() enters
the fail path. What I wanted is to make sdl2 works again and do not change the
code too much, hence I made sdl2_init() works as sdl2_write_header() with a
small change. I should look into the usage of inited carefully.

> 
> 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).

How about to fix sdl initialization and memory issue in a separate patch if
someone is interested ? 

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

Never mind and many thanks for your valuable comments. 

BRs
Haihao

> 
> > +        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:
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

> On Sep 19, 2023, at 14:32, Xiang, Haihao <haihao.xiang-at-intel.com@ffmpeg.org> wrote:
> 
> On Ma, 2023-09-18 at 14:00 +0200, Andreas Rheinhardt wrote:
>> Xiang, Haihao:
>>> From: Haihao Xiang <haihao.xiang@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. 
> 
> Seems calling sdl2_write_header() and sdl2_write_packet() in two different
> threads are allowed. I think the change in commit 2d924b3 revealed a bug in
> libavdevice, we should fix libavdevice.

SDL *must* be run in main thread on some platform like macOS. I don’t think it’s
practical to fix the issue inside libavdevice. Add some checks like wether write_header
and write packet run in the same thread can be helpful, but it doesn’t solve current
issue.


*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'API misuse warning: setting the main menu on a non-main thread. Main menu contents should only be modified from the main thread.'
*** First throw call stack:
(
        0   CoreFoundation                      0x00000001872c88c0 __exceptionPreprocess + 176
        1   libobjc.A.dylib                     0x0000000186dc1eb4 objc_exception_throw + 60
        2   Foundation                          0x000000018840f18c -[NSCalendarDate initWithCoder:] + 0
        3   AppKit                              0x000000018aa0a444 -[NSMenu _setMenuName:] + 488
        4   AppKit                              0x000000018aa1efe8 -[NSApplication setMainMenu:] + 372
        5   libSDL2-2.0.0.dylib                 0x00000001044c9704 Cocoa_RegisterApp + 420
        6   libSDL2-2.0.0.dylib                 0x00000001044cf124 Cocoa_CreateDevice + 28
        7   libSDL2-2.0.0.dylib                 0x00000001044b7084 SDL_VideoInit_REAL + 464
        8   libSDL2-2.0.0.dylib                 0x000000010442787c SDL_InitSubSystem_REAL + 204

> 
> 
>> This patch makes things worse for people who want
>> their call to avformat_write_header() to actually check whether sdl2 works.
> 
> sdl2_write_header() always returns 0 (you also pointed it out below), so this
> change doesn't have impact to user if user want to use avformat_write_header()
> to check whether sdl2 works. 
> 
>> 
>>> 
>>> $ ffmpeg -f lavfi -i yuvtestsrc -pix_fmt yuv420p -f sdl2 "sdl2"
>>> 
>>> Signed-off-by: Haihao Xiang <haihao.xiang@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.
> 
> Thanks for pointing it out, I will delete it in the next version if you agree we
> should fix the library. 
> 
>> 
>>> +
>>> +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.
> 
> Honestly I am not an sdl2 expert too. I noticed sdl2_write_header() always
> return 0, and sdl2_write_packet() is called even if sdl2_write_header() enters
> the fail path. What I wanted is to make sdl2 works again and do not change the
> code too much, hence I made sdl2_init() works as sdl2_write_header() with a
> small change. I should look into the usage of inited carefully.
> 
>> 
>> 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).
> 
> How about to fix sdl initialization and memory issue in a separate patch if
> someone is interested ? 
> 
>> 
>> Sorry for this rant which does not affect your threading issue at all.
> 
> Never mind and many thanks for your valuable comments. 
> 
> BRs
> Haihao
> 
>> 
>>> +        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:
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".