[vlc-devel] [PATCH] RFC: avcodec: use a mutex instead of sem
Rémi Denis-Courmont
remi at remlab.net
Wed Sep 25 19:30:42 CEST 2019
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.
> @@ -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.
> 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/
More information about the vlc-devel
mailing list