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

["Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org>]

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

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