[FFmpeg-devel] [PATCH] lavd/sdl: add event handler thread

[Lukasz M]

On 25 November 2013 00:30, Stefano Sabatini <stefasab at gmail.com> wrote:

> On date Sunday 2013-11-24 20:36:28 +0100, Lukasz M encoded:
> > On 24 November 2013 20:12, Stefano Sabatini <stefasab at gmail.com> wrote:
> >
> > > Experimental and partially untested, meant to address ticket #1743 and
> > > #1744.
> > > ---
> > >  libavdevice/sdl.c | 115
> > > +++++++++++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 96 insertions(+), 19 deletions(-)
> > >
> > > +static int event_thread(void *arg)
> > > +{
> > > +    AVFormatContext *s = arg;
> > > +    SDLContext *sdl = s->priv_data;
> > > +    int flags = SDL_SWSURFACE | sdl->window_fullscreen ?
> SDL_FULLSCREEN :
> > > 0;
> > > +    AVStream *st = s->streams[0];
> > > +    AVCodecContext *encctx = st->codec;
> > > +
> > > +    /* initialization */
> > > +    SDL_WM_SetCaption(sdl->window_title, sdl->icon_title);
> > > +    sdl->surface = SDL_SetVideoMode(sdl->window_width,
> sdl->window_height,
> > > +                                    24, flags);
> > > +    if (!sdl->surface) {
> > > +        av_log(sdl, AV_LOG_ERROR, "Unable to set video mode: %s\n",
> > > SDL_GetError());
> > > +        return AVERROR(EINVAL);
> > >
> >
> > You will have a deadlock, sdl->inited is never set to 1 and
> > sdl_write_header will wait forever. The same below.
> > Returned values are send to nowhere, but no clue if they can be handled.
> > Setting sdl->quit to 1 and signal sdl->init_event_cond may help.
> > In sdl_write_header you may check if quit is set and return with error.
>
> Should be fixed.
>

Not tested, but looks OK.


>
> >
> > > +    }
> > > +
> > > +    sdl->overlay = SDL_CreateYUVOverlay(encctx->width, encctx->height,
> > > +                                        sdl->overlay_fmt,
> sdl->surface);
> > > +    if (!sdl->overlay || sdl->overlay->pitches[0] < encctx->width) {
> > > +        av_log(s, AV_LOG_ERROR,
> > > +               "SDL does not support an overlay with size of %dx%d
> > > pixels\n",
> > > +               encctx->width, encctx->height);
> > > +        return AVERROR(EINVAL);
> > > +    }
> > > +
> > > +    av_log(s, AV_LOG_VERBOSE, "w:%d h:%d fmt:%s -> w:%d h:%d\n",
> > > +           encctx->width, encctx->height,
> > > av_get_pix_fmt_name(encctx->pix_fmt),
> > > +           sdl->overlay_width, sdl->overlay_height);
> > > +
>
> > > +    SDL_LockMutex(sdl->mutex);
> > > +    sdl->inited = 1;
> > > +    SDL_CondSignal(sdl->init_event_cond);
> > > +    SDL_UnlockMutex(sdl->mutex);
> > >
> >
> > You can unlock mutex before signaling.
>
> Does it make any difference? I based the code on the SDL wiki example.
>

Its not an error for sure so feel free to ignore, but SDL_CondWait will
have to lock this mutex at quiting and it is still locked here for no
reason, so there may be not significant delay.


> One thing I don't like very much about the patch, is that it is not
> clear how to signal that the "output ended". I'm doing it by sending
> an EOF when quit is created, but this will cause an error and a
> failure from ffmpeg (we should probably handle the case, and don't
> treat EOF as an error).
>

I saw, but I am not sure this is not what should be expected.
It can be compared oo usb flash taken out of port while ffmpeg is writing
to it.
Not checked though what is happening in this case.
I was just surprised you picked EOF, and not AVERROR(EIO) for example.