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

[Marton Balint]

On Sun, 28 Aug 2011, avcoder wrote:

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

'Code b' only prevents the lock in the aborted thread. It does not prevent 
the lock when a new video_thread is spawned, and videoq.abort_request is 
no longer set. In the freshly spawned video_thread the code execution goes 
through 'code a', because the alloc event from the aborted thread 
may only be processed after posting of the second alloc event.

Regards,
   Marton

>
>
> -- 
> -----------------------------------------------------------------------------------------
> My key fingerprint: d1:03:f5:32:26:ff:d7:3c:e4:42:e3:51:ec:92:78:b2
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>