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

[avcoder]

On Sun, Aug 28, 2011 at 4:13 AM, Marton Balint <cus at passwd.hu> wrote:
>>>
>>> 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.

Thanks for your patient.

I understand your conclusion, 'stream_cycle_channel()' could ( only? )
cause this bug.
Your patch is valid.

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