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

[Zhao Zhili <quinkblack@foxmail.com>]

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

_______________________________________________
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".