[FFmpeg-devel] [PATCH WIPv2 2/2] avdev: add sdl2 device

[Marton Balint]

On Sat, 21 May 2016, Josh de Kock wrote:

> Hi Marton,
>
> Sorry for not updating it after we last spoke, I lost my source for the
> patch so I was force to do it again. Anyways, here it is. I'm using
> SDL_Events for sending the packets, using SDL_WaitEvent to throttle it,
> and Conditions to make sure that the queue doesn't build up. Is there
> anything I missed?

I think SDL_Init needs to be the first SDL function you call. You may try 
doing only that in write_header, to see if it works. Similarly, you may 
try moving SDL_Quit to write_trailer after joining the event 
thread and destorying all mutexes.

Some more comments below...

[...]

> +            case SDL_QUIT:
> +                sdl->quit = 1;
> +                break;
> +            case SDL_WINDOWEVENT:
> +                switch(event.window.event){
> +                    case SDL_WINDOWEVENT_RESIZED:
> +                    case SDL_WINDOWEVENT_SIZE_CHANGED:
> +                        sdl->window_width  = event.window.data1;
> +                        sdl->window_height = event.window.data2;
> +                        SDL_LockMutex(sdl->mutex);

This lock is no longer necessary

> +                        compute_texture_rect(s);
> +                        SDL_UnlockMutex(sdl->mutex);
> +                        break;
> +                    default:
> +                        break;
> +                }
> +                break;

[...]


> +static int sdl2_write_header(AVFormatContext *s)
> +{
> +    SDLContext *sdl = s->priv_data;
> +    AVCodecParameters *par = s->streams[0]->codecpar;
> +    int i, ret = 0;
> +
> +    if (!sdl->window_title)
> +        sdl->window_title = av_strdup(s->filename);
> +
> +    if (SDL_WasInit(SDL_INIT_VIDEO)) {
> +        av_log(s, AV_LOG_WARNING,
> +               "SDL video subsystem was already inited, you could have multiple SDL outputs. This may cause unknown behaviour.\n");
> +        sdl->init_ret = AVERROR(EINVAL);
> +        sdl->inited = 1;

Aren't you supposed to directly return error here? Multiple outputs are 
simply not supported. (Also strdup window_title after the initialization 
check, so you don't have to free it in case of error).

> +    }
> +
> +    if (   s->nb_streams > 1
> +        || par->codec_type != AVMEDIA_TYPE_VIDEO
> +        || par->codec_id   != AV_CODEC_ID_RAWVIDEO) {
> +        av_log(s, AV_LOG_ERROR, "Only supports one rawvideo stream\n");
> +        sdl->init_ret = AVERROR(EINVAL);
> +        goto fail;
> +    }
> +
> +    for (i = 0; sdl_texture_pix_fmt_map[i].pix_fmt != AV_PIX_FMT_NONE; i++) {
> +        if (sdl_texture_pix_fmt_map[i].pix_fmt == par->format) {
> +            sdl->texture_fmt = sdl_texture_pix_fmt_map[i].texture_fmt;
> +            break;
> +        }
> +    }
> +
> +    if (!sdl->texture_fmt) {
> +        av_log(s, AV_LOG_ERROR,
> +               "Unsupported pixel format '%s'\n",
> +               av_get_pix_fmt_name(par->format));
> +        sdl->init_ret = AVERROR(EINVAL);
> +        goto fail;
> +    }
> +
> +    sdl->condition = SDL_CreateCond();
> +    if (!sdl->condition) {
> +        av_log(s, AV_LOG_ERROR, "Could not create SDL condition variable: %s\n", SDL_GetError());
> +        ret = AVERROR_EXTERNAL;
> +        goto fail;
> +    }
> +    sdl->mutex = SDL_CreateMutex();
> +    if (!sdl->mutex) {
> +        av_log(s, AV_LOG_ERROR, "Could not create SDL mutex: %s\n", SDL_GetError());
> +        ret = AVERROR_EXTERNAL;
> +        goto fail;
> +    }
> +    sdl->event_thread = SDL_CreateThread(event_thread, "event_thread", s);
> +    if (!sdl->event_thread) {
> +        av_log(s, AV_LOG_ERROR, "Could not create SDL event thread: %s\n", SDL_GetError());
> +        ret = AVERROR_EXTERNAL;
> +        goto fail;
> +    }
> +
> +    /* Wait until the video system has been initiated. */
> +    SDL_LockMutex(sdl->mutex);
> +    while (!sdl->inited) {
> +        SDL_CondWait(sdl->condition, sdl->mutex);
> +    }
> +
> +    SDL_UnlockMutex(sdl->mutex);
> +    if (sdl->init_ret < 0) {
> +        ret = sdl->init_ret;
> +        goto fail;
> +    }
> +    return 0;
> +
> +fail:
> +    sdl2_write_trailer(s);

You should also free window_title in sdl2_write_trailer as far as I see.

> +    return ret;
> +}
> +
> +static int sdl2_write_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    SDLContext *sdl = s->priv_data;
> +    if(sdl->quit)
> +        return sdl2_write_trailer(s);

Calling write_trailer here is not necessary, the user should take care of 
that. You should simply return AVERROR(EIO) if the event thread has quit.

> +
> +    AVPacket *ref_pkt = av_packet_alloc();
> +    av_packet_ref(ref_pkt, pkt);
> +    SDL_Event event;
> +    event.type = FF_PACKET_EVENT;
> +    event.user.data1 = ref_pkt;
> +    SDL_PushEvent(&event);
> +
> +    SDL_LockMutex(sdl->mutex);
> +    while(!sdl->pkt_sent)
> +        SDL_CondWait(sdl->condition, sdl->mutex);

In order not to get locked up in this loop you should also handle the case 
when the event thread exited when you were waiting for the packet to reach 
it... You may also miss the signal, so I suggest you use CondWaitTimeout. 
Also you may have to free the packet reference somehow in this case 
yourself, probably in write_trailer after joining the event thread, so it 
probably make sense to store the packet reference in a context variable.

Regards,
Marton