[FFmpeg-devel] [PATCH] ffplay: fix a crash caused by aborting the video queue

[avcoder]

On Sun, Aug 28, 2011 at 1:26 AM, Marton Balint <cus at passwd.hu> wrote:
>
> On Sun, 28 Aug 2011, avcoder wrote:
>
>> On Sun, Aug 28, 2011 at 12:40 AM, Marton Balint <cus at passwd.hu> wrote:
>>>
>>> On Sat, 27 Aug 2011, avcoder wrote:
>>>
>>>> On Sat, Aug 27, 2011 at 6:09 PM, Marton Balint <cus at passwd.hu> wrote:
>>>>>
>>>>> Yes, the patch description is probably not entirely correct, sorry, my
>>>>> mistake. The thought of changing w_index somehow remained in my mind
>>>>> from
>>>>> the time I debugged this.
>>>>>
>>>>> The main problem with the code is that multiple ALLOC events may occur
>>>>> at
>>>>> the same time if we don't pop them from the event queue on abort. If
>>>>> these
>>>>> alloc events tries to allocate the same vp, vp->bmp changes (free-d,
>>>>> and
>>>>> the
>>>>> then allocated again), I typically got the crash when
>>>>> SDL_LockYUVOverlay(vp->bmp) was called from the video thread, after
>>>>> SDL_FreeYUVOverlay(vp->bmp) was called from the main thread.
>>>>
>>>> Why do you get multiple ALLOC envents?
>>>>
>>>> I checked the ALLOC events, FF_ALLOC_EVENT is only pushed in
>>>> "queue_picture()" which is called by "video_thread()", it is
>>>> impossible to have MULTIPLE FF_ALLOC_EVENT.
>>>
>>> When you abort the video queue, an ALLOC event may be in the event queue.
>>> When another video_thread() is stared (e.g. when changing the video
>>> stream)
>>> the event from the first video_thread may still be in the queue.
>>>
>>
>> OK, that's the point.
>>
>> But this scenario has no relationship with your previous explanation
>> -----'if these alloc events tries to allocate the same vp...., "
>> The vp is not same if you start another video_thread(). so there's no
>> scenario to produce your bug: "  I typically got the crash when
>> SDL_LockYUVOverlay(vp->bmp) was called from the video thread, after
>> SDL_FreeYUVOverlay(vp->bmp) was called from the main thread."
>>
>> Your patch is harmless, but useless.
>>
>
> I don't agree, because with my patch, alloc_picture and SDL_LockYUVOverlay
> of queue_picture cannot run at the same time, therefore there is no chance
> that SDL_LockYUVOverlay will do a lock using a currently freed pointer,
> which may be the case, if SDL_LockYUVOverlay happens when the main thread is
> in the middle of alloc_picture.


SDL_LockYUVOverlay has no chance to happens when the main thread is in
the middle of alloc_picture in the original implementation

there are serialization.

    /* alloc or resize hardware picture buffer */
    if (!vp->bmp ||
#if CONFIG_AVFILTER
        vp->width  != is->out_video_filter->inputs[0]->w ||
        vp->height != is->out_video_filter->inputs[0]->h) {
#else
        vp->width != is->video_st->codec->width ||
        vp->height != is->video_st->codec->height) {
#endif
        SDL_Event event;

        vp->allocated = 0;

        /* the allocation must be done in the main thread to avoid
           locking problems */
        event.type = FF_ALLOC_EVENT;
        event.user.data1 = is;
        SDL_PushEvent(&event);

        // ----------------------- code a--------------------------------------
        /* wait until the picture is allocated */
        SDL_LockMutex(is->pictq_mutex);
        while (!vp->allocated && !is->videoq.abort_request) {
            SDL_CondWait(is->pictq_cond, is->pictq_mutex);
        }
        SDL_UnlockMutex(is->pictq_mutex);

        // -----------------------code b --------------------------------------
        if (is->videoq.abort_request)
            return -1;
    }

    /* if the frame is not skipped, then display it */
    if (vp->bmp) {
        AVPicture pict;
#if CONFIG_AVFILTER
        if(vp->picref)
            avfilter_unref_buffer(vp->picref);
        vp->picref = src_frame->opaque;
#endif

        // ----------------------------code c-----------------------------------
        /* get a pointer on the bitmap */
        SDL_LockYUVOverlay (vp->bmp);

        memset(&pict,0,sizeof(AVPicture));
        pict.data[0] = vp->bmp->pixels[0];
        pict.data[1] = vp->bmp->pixels[2];
        pict.data[2] = vp->bmp->pixels[1];

        pict.linesize[0] = vp->bmp->pitches[0];
        pict.linesize[1] = vp->bmp->pitches[2];
        pict.linesize[2] = vp->bmp->pitches[1];

......

}


'code c' and 'alloc_picture()' can be concurrent only in abort mode,
but 'code b ' prevent them.


-- 
-----------------------------------------------------------------------------------------
My key fingerprint: d1:03:f5:32:26:ff:d7:3c:e4:42:e3:51:ec:92:78:b2