On Ma, 2023-09-18 at 14:00 +0200, Andreas Rheinhardt wrote:
> Xiang, Haihao:
> > From: Haihao Xiang <haihao.xi...@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.xi...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
