[vlc-devel] [PATCH] RFC: avcodec: use a mutex instead of sem
Thomas Guillem
thomas at gllm.fr
Thu Sep 26 09:48:28 CEST 2019
On Thu, Sep 26, 2019, at 09:13, Thomas Guillem wrote:
>
>
> 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.
Ah no, these tsan reports were fixed by my fix avcodec patch: "avcodec: fix data-race" Where I moved up some post_mt() in DecodeBlock() to protect access to the pts variable for example.
>
> 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
> _______________________________________________
> 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