[FFmpeg-devel] [PATCH] lavd/sdl2: postpone sdl2 window creation
Zhao Zhili
quinkblack at foxmail.com
Mon Oct 9 10:08:34 EEST 2023
> On Sep 19, 2023, at 14:32, Xiang, Haihao <haihao.xiang-at-intel.com at ffmpeg.org> wrote:
>
> On Ma, 2023-09-18 at 14:00 +0200, Andreas Rheinhardt wrote:
>> 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.
>
> 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 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.
>
> 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list