[vlc-devel] [PATCH] RFC: avcodec: use a mutex instead of sem
Thomas Guillem
thomas at gllm.fr
Thu Sep 26 09:13:44 CEST 2019
On Wed, Sep 25, 2019, at 19:30, Rémi Denis-Courmont wrote:
> Le keskiviikkona 25. syyskuuta 2019, 16.38.05 EEST Thomas Guillem a écrit :
> > RFC: This commit has been tested with/without va, on some of my samples. It
> > could needs a more extensive testing. Do you some corner case for me to test
> > ?
> >
> > Using a mutex instead of a sem allows avcodec callbacks to not wait for a
> > future pf_decode and pf_flush call.
> >
> > The mutex protect the pts, the p_va, the dec->fmt_out variables, the
> > decoder_UpdateVideoFormat() and the decoder_NewPicture() call. Indeed,
> > decoder_UpdateVideoFormat() is not reentrant and the caller should take care
> > about serialization.
> >
> > This fixes the following 3 data-race (happening with/without va, and while
> > seeking...):
>
> I don't see how switching from semaphore to mutex can fix any data race as
> such. Unless the patch silently changes something else of course - which seems
> to be the case here.
Yes, after writing that patch, I wondered the same. Either tsan is sending false positives when using semaphore or I fixed something I don't really understand yet.
Anyway, after second though, I think semaphore were used because ffmpeg was not thread-safe in that time. Indeed, the additional goal of the semaphore was to prevent any concurrent avcodec call. After reading the avcodec documentation, we don't need that anymore.
>
> > @@ -722,11 +712,9 @@ static void Flush( decoder_t *p_dec )
> > * and workers threads */
> > decoder_AbortPictures( p_dec, true );
> >
> > - post_mt( p_sys );
> > /* do not flush buffers if codec hasn't been opened (theora/vorbis/VC1)
> > */ if( avcodec_is_open( p_context ) )
> > avcodec_flush_buffers( p_context );
> > - wait_mt( p_sys );
> >
> > /* Reset cancel state to false */
> > decoder_AbortPictures( p_dec, false );
> > @@ -1071,8 +1059,6 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) {
> > int i_used = 0;
> >
> > - post_mt( p_sys );
> > -
> > if( (p_block && p_block->i_buffer > 0) || b_drain )
> > {
> > AVPacket pkt;
> > @@ -1159,8 +1145,6 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) }
> > bool not_received_frame = ret;
> >
> > - wait_mt( p_sys );
> > -
> > if( p_block )
> > {
> > /* Consumed bytes */
> > @@ -1180,6 +1164,8 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) if( p_frame_info->b_eos )
> > p_sys->b_first_frame = true;
> >
> > + vlc_mutex_lock(&p_sys->lock);
> > +
> > /* Compute the PTS */
> > #ifdef FF_API_PKT_PTS
> > int64_t av_pts = frame->pts == AV_NOPTS_VALUE ? frame->pkt_dts :
> > frame->pts; @@ -1207,6 +1193,7 @@ static int DecodeBlock( decoder_t *p_dec,
> > block_t **pp_block ) !p_sys->b_show_corrupted ) )
> > {
> > av_frame_free(&frame);
> > + vlc_mutex_unlock(&p_sys->lock);
>
> AFAIK, this can be done before av_frame_free() and picture_Release() calls.
Yes
>
> > continue;
> > }
> >
> > @@ -1225,6 +1212,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) {
> > b_error = true;
> > av_frame_free(&frame);
> > + vlc_mutex_unlock(&p_sys->lock);
> > break;
> > }
> > static_assert( sizeof(p_palette->palette) == AVPALETTE_SIZE,
> > @@ -1236,6 +1224,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) if( decoder_UpdateVideoFormat( p_dec ) )
> > {
> > av_frame_free(&frame);
> > + vlc_mutex_unlock(&p_sys->lock);
> > continue;
> > }
> > }
> > @@ -1253,6 +1242,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) if( !p_pic )
> > {
> > av_frame_free(&frame);
> > + vlc_mutex_unlock(&p_sys->lock);
> > break;
> > }
> >
> > @@ -1261,6 +1251,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) {
> > av_frame_free(&frame);
> > picture_Release( p_pic );
> > + vlc_mutex_unlock(&p_sys->lock);
> > break;
> > }
> > }
> > @@ -1275,6 +1266,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) if( unlikely(p_pic == NULL) )
> > {
> > av_frame_free(&frame);
> > + vlc_mutex_unlock(&p_sys->lock);
> > break;
> > }
> > }
> > @@ -1312,11 +1304,14 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block ) if(p_frame_info->b_eos)
> > p_pic->b_still = true;
> > p_sys->b_first_frame = false;
> > + vlc_mutex_unlock(&p_sys->lock);
> > decoder_QueueVideo( p_dec, p_pic );
> > }
> > else
> > + {
> > + vlc_mutex_unlock(&p_sys->lock);
> > picture_Release( p_pic );
> > -
> > + }
> > } while( true );
> >
> > /* After draining, we need to reset decoder with a flush */
> > @@ -1369,14 +1364,10 @@ void EndVideoDec( vlc_object_t *obj )
> > decoder_sys_t *p_sys = p_dec->p_sys;
> > AVCodecContext *ctx = p_sys->p_context;
> >
> > - post_mt( p_sys );
> > -
> > /* do not flush buffers if codec hasn't been opened (theora/vorbis/VC1)
> > */ if( avcodec_is_open( ctx ) )
> > avcodec_flush_buffers( ctx );
> >
> > - wait_mt( p_sys );
> > -
> > cc_Flush( &p_sys->cc );
> >
> > avcodec_free_context( &ctx );
> > @@ -1384,7 +1375,7 @@ void EndVideoDec( vlc_object_t *obj )
> > if( p_sys->p_va )
> > vlc_va_Delete( p_sys->p_va );
> >
> > - vlc_sem_destroy( &p_sys->sem_mt );
> > + vlc_mutex_destroy( &p_sys->lock );
> > free( p_sys );
> > }
> >
> > @@ -1583,12 +1574,12 @@ static int lavc_GetFrame(struct AVCodecContext *ctx,
> > AVFrame *frame, int flags) }
> > frame->opaque = NULL;
> >
> > - wait_mt(sys);
> > + vlc_mutex_lock(&sys->lock);
> > if (sys->p_va == NULL)
> > {
> > if (!sys->b_direct_rendering)
> > {
> > - post_mt(sys);
> > + vlc_mutex_unlock(&sys->lock);
> > return avcodec_default_get_buffer2(ctx, frame, flags);
> > }
> >
> > @@ -1597,18 +1588,22 @@ static int lavc_GetFrame(struct AVCodecContext *ctx,
> > AVFrame *frame, int flags) * to protect p_dec->fmt_out. */
> > if (lavc_UpdateVideoFormat(dec, ctx, ctx->pix_fmt, ctx->pix_fmt))
> > {
> > - post_mt(sys);
> > + vlc_mutex_unlock(&sys->lock);
> > return -1;
> > }
> > }
> > - post_mt(sys);
> >
> > if (sys->p_va != NULL)
> > - return lavc_va_GetFrame(ctx, frame);
> > + {
> > + int ret = lavc_va_GetFrame(ctx, frame);
> > + vlc_mutex_unlock(&sys->lock);
> > + return ret;
> > + }
> >
> > /* Some codecs set pix_fmt only after the 1st frame has been decoded,
> > * so we need to check for direct rendering again. */
> > int ret = lavc_dr_GetFrame(ctx, frame);
> > + vlc_mutex_unlock(&sys->lock);
> > if (ret)
> > ret = avcodec_default_get_buffer2(ctx, frame, flags);
> > return ret;
> > @@ -1696,7 +1691,7 @@ no_reuse:
> > }
> > #endif
> >
> > - wait_mt(p_sys);
> > + vlc_mutex_lock(&p_sys->lock);
> >
> > static const enum PixelFormat hwfmts[] =
> > {
> > @@ -1731,29 +1726,28 @@ no_reuse:
> > msg_Dbg(p_dec, "trying format %s", dsc ? dsc->name : "unknown");
> > if (lavc_UpdateVideoFormat(p_dec, p_context, hwfmt, swfmt))
> > continue; /* Unsupported brand of hardware acceleration */
> > - post_mt(p_sys);
> > + vlc_mutex_unlock(&p_sys->lock);
> >
> > picture_t *test_pic = decoder_NewPicture(p_dec);
> > assert(!test_pic || test_pic->format.i_chroma ==
> > p_dec->fmt_out.video.i_chroma); vlc_va_t *va =
> > vlc_va_New(VLC_OBJECT(p_dec), p_context, hwfmt, &p_dec->fmt_in,
> > test_pic ? test_pic->p_sys : NULL);
> > + vlc_mutex_lock(&p_sys->lock);
> > if (test_pic)
> > picture_Release(test_pic);
> > if (va == NULL)
> > - {
> > - wait_mt(p_sys);
> > continue; /* Unsupported codec profile or such */
> > - }
> >
> > p_sys->p_va = va;
> > p_sys->pix_fmt = hwfmt;
> > p_context->draw_horiz_band = NULL;
> > + vlc_mutex_unlock(&p_sys->lock);
> > return hwfmt;
> > }
> >
> > - post_mt(p_sys);
> > /* Fallback to default behaviour */
> > p_sys->pix_fmt = swfmt;
> > + vlc_mutex_unlock(&p_sys->lock);
> > return swfmt;
> > }
>
>
> --
> Реми Дёни-Курмон
> http://www.remlab.net/
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list