[vlc-devel] [PATCH 11/11] input: move vout start/stop handling in resource
Thomas Guillem
thomas at gllm.fr
Wed Jul 1 11:10:16 CEST 2020
On Wed, Jul 1, 2020, at 11:00, Steve Lhomme wrote:
> Comments below.
>
> On 2020-06-30 17:27, Thomas Guillem wrote:
> > The start state is now know by the input resource and protected with its lock.
> > Move vout_StopDisplay() and vout_ChangeSource() to input/resource.c since the
> > execution of these functions depend on the start state.
> > ---
> > src/input/decoder.c | 51 +++++++++++++++++---------------------------
> > src/input/resource.c | 41 ++++++++++++++++++++++++++++++-----
> > src/input/resource.h | 5 +++--
> > 3 files changed, 59 insertions(+), 38 deletions(-)
> >
> > diff --git a/src/input/decoder.c b/src/input/decoder.c
> > index ed7cb6803b8..ec3711cc685 100644
> > --- a/src/input/decoder.c
> > +++ b/src/input/decoder.c
> > @@ -128,7 +128,6 @@ struct vlc_input_decoder_t
> >
> > vout_thread_t *p_vout;
> > enum vlc_vout_order vout_order;
> > - bool vout_thread_started;
> >
> > /* -- Theses variables need locking on read *and* write -- */
> > /* Preroll */
> > @@ -463,26 +462,21 @@ static int ModuleThread_UpdateVideoFormat( decoder_t *p_dec, vlc_video_context *
> > return -1;
> > }
> > }
> > - if (p_owner->vout_thread_started)
> > - {
> > - int res = vout_ChangeSource(p_owner->p_vout, &p_dec->fmt_out.video);
> > - if (res == 0)
> > - // the display/thread is started and can handle the new source format
> > - return 0;
> > - }
> >
> > vout_configuration_t cfg = {
> > .vout = p_owner->p_vout, .clock = p_owner->p_clock, .fmt = &p_dec->fmt_out.video,
> > .mouse_event = MouseEvent, .mouse_opaque = p_dec,
> > };
> > + bool started;
> > vout_thread_t *p_vout =
> > - input_resource_RequestVout(p_owner->p_resource, vctx, &cfg, NULL);
> > + input_resource_RequestVout(p_owner->p_resource, vctx, &cfg, NULL, &started);
> > if (p_vout != NULL)
> > {
> > - p_owner->vout_thread_started = true;
> > - decoder_Notify(p_owner, on_vout_started, p_vout, p_owner->vout_order);
> > + if (started)
> > + decoder_Notify(p_owner, on_vout_started, p_vout, p_owner->vout_order);
> > return 0;
> > }
> > +
> > return -1;
> > }
> >
> > @@ -543,7 +537,7 @@ static int CreateVoutIfNeeded(vlc_input_decoder_t *p_owner)
> >
> > enum vlc_vout_order order;
> > const vout_configuration_t cfg = { .vout = p_vout, .fmt = NULL };
> > - p_vout = input_resource_RequestVout( p_owner->p_resource, NULL, &cfg, &order );
> > + p_vout = input_resource_RequestVout( p_owner->p_resource, NULL, &cfg, &order, NULL );
> >
> > vlc_mutex_lock( &p_owner->lock );
> > p_owner->p_vout = p_vout;
> > @@ -663,7 +657,6 @@ static subpicture_t *ModuleThread_NewSpuBuffer( decoder_t *p_dec,
> >
> > vout_Release(p_owner->p_vout);
> > p_owner->p_vout = NULL; // the DecoderThread should not use the old vout anymore
> > - p_owner->vout_thread_started = false;
> > vlc_mutex_unlock( &p_owner->lock );
> > }
> > return NULL;
> > @@ -684,7 +677,6 @@ static subpicture_t *ModuleThread_NewSpuBuffer( decoder_t *p_dec,
> > p_owner->i_spu_channel);
> > vout_Release(p_owner->p_vout);
> > p_owner->p_vout = NULL; // the DecoderThread should not use the old vout anymore
> > - p_owner->vout_thread_started = false;
> > }
> >
> > enum vlc_vout_order channel_order;
> > @@ -702,7 +694,6 @@ static subpicture_t *ModuleThread_NewSpuBuffer( decoder_t *p_dec,
> > }
> >
> > p_owner->p_vout = p_vout;
> > - p_owner->vout_thread_started = true;
> > p_owner->vout_order = channel_order;
> > vlc_mutex_unlock(&p_owner->lock);
> >
> > @@ -1021,7 +1012,7 @@ static void ModuleThread_QueueCc( decoder_t *p_videodec, block_t *p_cc,
> > static int ModuleThread_PlayVideo( vlc_input_decoder_t *p_owner, picture_t *p_picture )
> > {
> > decoder_t *p_dec = &p_owner->dec;
> > - vout_thread_t *p_vout = p_owner->vout_thread_started ? p_owner->p_vout : NULL;
> > + vout_thread_t *p_vout = p_owner->p_vout;
> >
> > if( p_picture->date == VLC_TICK_INVALID )
> > /* FIXME: VLC_TICK_INVALID -- verify video_output */
> > @@ -1482,12 +1473,12 @@ static void DecoderThread_Flush( vlc_input_decoder_t *p_owner )
> > }
> > else if( p_dec->fmt_out.i_cat == VIDEO_ES )
> > {
> > - if( p_owner->p_vout && p_owner->vout_thread_started )
> > + if( p_owner->p_vout )
> > vout_FlushAll( p_owner->p_vout );
> > }
> > else if( p_dec->fmt_out.i_cat == SPU_ES )
> > {
> > - if( p_owner->p_vout && p_owner->vout_thread_started )
> > + if( p_owner->p_vout )
> > {
> > assert( p_owner->i_spu_channel != VOUT_SPU_CHANNEL_INVALID );
> > vout_FlushSubpictureChannel( p_owner->p_vout, p_owner->i_spu_channel );
> > @@ -1796,7 +1787,6 @@ CreateDecoder( vlc_object_t *p_parent,
> > p_owner->cbs_userdata = cbs_userdata;
> > p_owner->p_aout = NULL;
> > p_owner->p_vout = NULL;
> > - p_owner->vout_thread_started = false;
> > p_owner->i_spu_channel = VOUT_SPU_CHANNEL_INVALID;
> > p_owner->i_spu_order = 0;
> > p_owner->p_sout = p_sout;
> > @@ -1973,15 +1963,16 @@ static void DeleteDecoder( vlc_input_decoder_t *p_owner )
> >
> > if (vout != NULL)
> > {
> > - if( p_owner->vout_thread_started)
> > - {
> > - /* Reset the cancel state that was set before joining the
> > - * decoder thread */
> > - vout_StopDisplay(vout);
> > - p_owner->vout_thread_started = false;
> > + /* Hold the vout since PutVout will likely release it and a
> > + * last reference is needed for notify callbacks */
> > + vout_Hold(vout);
> > +
> > + bool stopped;
> > + input_resource_PutVout(p_owner->p_resource, vout, &stopped);
> > + if (stopped)
> > decoder_Notify(p_owner, on_vout_stopped, vout);
> > - }
> > - input_resource_PutVout(p_owner->p_resource, vout);
> > +
> > + vout_Release(vout);
> > }
> > break;
> > }
> > @@ -1995,7 +1986,6 @@ static void DeleteDecoder( vlc_input_decoder_t *p_owner )
> > vout_UnregisterSubpictureChannel( p_owner->p_vout,
> > p_owner->i_spu_channel );
> > vout_Release(p_owner->p_vout);
> > - p_owner->vout_thread_started = false;
> > }
> > break;
> > }
> > @@ -2157,7 +2147,7 @@ void vlc_input_decoder_Delete( vlc_input_decoder_t *p_owner )
> > *
> > * This unblocks the thread, allowing the decoder module to join all its
> > * worker threads (if any) and the decoder thread to terminate. */
> > - if( p_dec->fmt_in.i_cat == VIDEO_ES && p_owner->p_vout != NULL && p_owner->vout_thread_started )
> > + if( p_dec->fmt_in.i_cat == VIDEO_ES && p_owner->p_vout != NULL )
> > {
> > if (p_owner->out_pool)
> > picture_pool_Cancel( p_owner->out_pool, true );
> > @@ -2303,8 +2293,7 @@ void vlc_input_decoder_Flush( vlc_input_decoder_t *p_owner )
> > * after being unstuck. */
> >
> > vlc_mutex_lock( &p_owner->lock );
> > - if( p_owner->dec.fmt_out.i_cat == VIDEO_ES
> > - && p_owner->p_vout && p_owner->vout_thread_started )
> > + if( p_owner->dec.fmt_out.i_cat == VIDEO_ES && p_owner->p_vout )
> > vout_FlushAll( p_owner->p_vout );
> > vlc_mutex_unlock( &p_owner->lock );
> > }
> > diff --git a/src/input/resource.c b/src/input/resource.c
> > index b82d04a2897..04d554c1d48 100644
> > --- a/src/input/resource.c
> > +++ b/src/input/resource.c
> > @@ -48,6 +48,7 @@ struct vout_resource
> > {
> > vout_thread_t *vout;
> > enum vlc_vout_order order;
> > + bool started;
> >
> > struct vlc_list node;
> > };
> > @@ -105,6 +106,7 @@ vout_resource_Create(vout_thread_t *vout)
> > if (unlikely(vout_rsc == NULL))
> > return NULL;
> >
> > + vout_rsc->started = false;
> > vout_rsc->vout = vout;
> > return vout_rsc;
> > }
> > @@ -376,12 +378,21 @@ void input_resource_SetInput( input_resource_t *p_resource, input_thread_t *p_in
> > }
> >
> > static void input_resource_PutVoutLocked(input_resource_t *p_resource,
> > - vout_thread_t *vout)
> > + vout_thread_t *vout, bool *stopped)
> > {
> > assert(vout != NULL);
> > struct vout_resource *vout_rsc = resource_GetVoutRsc(p_resource, vout);
> > assert(vout_rsc != NULL);
> >
> > + if (stopped != NULL)
> > + *stopped = vout_rsc->started;
>
> I think the variable should be "was_stopped" or "has_stopped" because it
> doesn't report the state but whether the call stopped it or not.
Indeed, I prefer "has_stopped".
>
> Otherwise *stopped should be !vout_rsc->started.
>
> > +
> > + if (vout_rsc->started)
> > + {
> > + vout_StopDisplay(vout_rsc->vout);
> > + vout_rsc->started = false;
> > + }
> > +
> > if (vout_rsc == resource_GetFirstVoutRsc(p_resource))
> > {
> > assert(p_resource->vout_rsc_free == NULL || p_resource->vout_rsc_free == vout_rsc);
> > @@ -403,10 +414,10 @@ static void input_resource_PutVoutLocked(input_resource_t *p_resource,
> > }
> >
> > void input_resource_PutVout(input_resource_t *p_resource,
> > - vout_thread_t *vout)
> > + vout_thread_t *vout, bool *stopped)
> > {
> > vlc_mutex_lock( &p_resource->lock );
> > - input_resource_PutVoutLocked( p_resource, vout );
> > + input_resource_PutVoutLocked( p_resource, vout, stopped );
> > vlc_mutex_unlock( &p_resource->lock );
> > }
> >
> > @@ -455,11 +466,15 @@ RequestVoutRsc(input_resource_t *p_resource)
> > vout_thread_t *input_resource_RequestVout(input_resource_t *p_resource,
> > vlc_video_context *vctx,
> > const vout_configuration_t *cfg,
> > - enum vlc_vout_order *order)
> > + enum vlc_vout_order *order,
> > + bool *started)
> > {
> > vlc_mutex_lock( &p_resource->lock );
> > struct vout_resource *vout_rsc = NULL;
> >
> > + if (started != NULL)
> > + *started = false;
> > +
> > vout_configuration_t dcfg = *cfg;
> > if (dcfg.vout == NULL)
> > {
> > @@ -490,8 +505,19 @@ vout_thread_t *input_resource_RequestVout(input_resource_t *p_resource,
> > return dcfg.vout;
> > }
> >
> > + if (vout_rsc->started)
> > + {
> > + assert(cfg->vout != NULL);
> > + int ret = vout_ChangeSource(dcfg.vout, dcfg.fmt);
> > + if (ret == 0)
> > + {
> > + vlc_mutex_unlock(&p_resource->lock);
> > + return dcfg.vout;
>
> Here the call doesn't report the started state properly. It says it's
> not started even though it may be started.
It is expected : the vout was already started. The started/stopped state is only used by the caller to forward vout start/stop notification.
>
> > + }
> > + }
> > +
> > if (vout_Request(&dcfg, vctx, p_resource->p_input)) {
> > - input_resource_PutVoutLocked(p_resource, dcfg.vout);
> > + input_resource_PutVoutLocked(p_resource, dcfg.vout, NULL);
> > vlc_mutex_unlock(&p_resource->lock);
> > return NULL;
> > }
> > @@ -505,7 +531,12 @@ vout_thread_t *input_resource_RequestVout(input_resource_t *p_resource,
> > input_ControlPush(p_resource->p_input, INPUT_CONTROL_SET_INITIAL_VIEWPOINT,
> > ¶m);
> > }
> > + vout_rsc->started = true;
>
> You could move this just after the successful vout_Request() call to
> make it clearer why it's started at this point.
OK
>
> > vlc_mutex_unlock( &p_resource->lock );
> > +
> > + if (started != NULL)
> > + *started = true;
> > +
> > return dcfg.vout;
> > }
> >
> > diff --git a/src/input/resource.h b/src/input/resource.h
> > index 7a725aabcf6..219049ee9b8 100644
> > --- a/src/input/resource.h
> > +++ b/src/input/resource.h
> > @@ -39,8 +39,9 @@ sout_instance_t *input_resource_RequestSout( input_resource_t *, sout_instance_t
> >
> > vout_thread_t *input_resource_RequestVout(input_resource_t *, vlc_video_context *,
> > const vout_configuration_t *,
> > - enum vlc_vout_order *order);
> > -void input_resource_PutVout(input_resource_t *, vout_thread_t *);
> > + enum vlc_vout_order *order,
> > + bool *started);
> > +void input_resource_PutVout(input_resource_t *, vout_thread_t *, bool *stopped);
> >
> > /**
> > * This function returns one of the current vout if any.
> > --
> > 2.20.1
> >
> > _______________________________________________
> > 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