[vlc-devel] [PATCH 1/4] video_output: move the window lock outside of the window enabling function

Thomas Guillem thomas at gllm.fr
Mon Jan 13 14:51:48 CET 2020


Both way are fine with me. I think the most important is to respect the style used by the file/struct/API.

On Mon, Jan 13, 2020, at 14:03, Steve Lhomme wrote:
> This is not the first time we have this discussion. This is exactly how 
> we name functions that are called with the locked done outside:
> 
> #find vlc -name "*.c" -exec grep -nH "Locked(" {} \;
> vlc/modules/access/rist.c:154:static void 
> rist_WriteTo_i11e_Locked(vlc_mutex_t lock, int fd, const void *buf, 
> size_t len,
> vlc/modules/access/rist.c:263:    rist_WriteTo_i11e_Locked(p_sys->lock, 
> flow->fd_nack, buf, rtcp_feedback_size,
> vlc/modules/access/rist.c:302: 
> rist_WriteTo_i11e_Locked(p_sys->lock, fd_nack, buf, len,
> vlc/modules/access/rist.c:342: 
> rist_WriteTo_i11e_Locked(p_sys->lock, fd_nack, buf, len,
> vlc/modules/audio_output/coreaudio_common.c:239:ca_GetLatencyLocked(audio_output_t 
> *p_aout)
> vlc/modules/audio_output/coreaudio_common.c:266:    *delay = 
> ca_GetLatencyLocked(p_aout) + i_render_delay;
> vlc/modules/audio_output/coreaudio_common.c:326:        const 
> vlc_tick_t 
> first_render_time = date - ca_GetLatencyLocked(p_aout);
> vlc/modules/audio_output/mmdevice.c:175:static int 
> VolumeSetLocked(audio_output_t *aout, float vol)
> vlc/modules/audio_output/mmdevice.c:200:    int ret = 
> VolumeSetLocked(aout, vol);
> vlc/modules/audio_output/mmdevice.c:733:static int 
> DeviceRequestLocked(audio_output_t *aout)
> vlc/modules/audio_output/mmdevice.c:750:static int 
> DeviceSelectLocked(audio_output_t *aout, const char *id)
> vlc/modules/audio_output/mmdevice.c:764:    return 
> DeviceRequestLocked(aout);
> vlc/modules/audio_output/mmdevice.c:767:static int 
> DeviceRestartLocked(audio_output_t *aout)
> vlc/modules/audio_output/mmdevice.c:773:    return 
> DeviceRequestLocked(aout);
> vlc/modules/audio_output/mmdevice.c:780:    int ret = 
> DeviceSelectLocked(aout, id);
> vlc/modules/audio_output/mmdevice.c:1137:    if 
> ((sys->request_device_restart && DeviceRestartLocked(aout) != 0)
> vlc/modules/audio_output/mmdevice.c:1172:            ret = 
> DeviceRestartLocked(aout);
> vlc/modules/audio_output/mmdevice.c:1179:            ret = 
> DeviceSelectLocked(aout, NULL);
> vlc/modules/audio_output/mmdevice.c:1264:        VolumeSetLocked(aout, 
> var_InheritFloat(aout, "mmdevice-volume"));
> vlc/modules/codec/omxil/mediacodec.c:168:static void 
> DecodeFlushLocked(decoder_t *);
> vlc/modules/codec/omxil/mediacodec.c:995:static void 
> AbortDecoderLocked(decoder_t *p_dec)
> vlc/modules/codec/omxil/mediacodec.c:1030:    DecodeFlushLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1032:    AbortDecoderLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1310:static void 
> DecodeFlushLocked(decoder_t *p_dec)
> vlc/modules/codec/omxil/mediacodec.c:1326:        
> AbortDecoderLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1341:    DecodeFlushLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1459:static int 
> QueueBlockLocked(decoder_t *p_dec, block_t *p_in_block,
> vlc/modules/codec/omxil/mediacodec.c:1579: 
> AbortDecoderLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1587:    AbortDecoderLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1612: 
> QueueBlockLocked(p_dec, NULL, true);
> vlc/modules/codec/omxil/mediacodec.c:1619: 
> QueueBlockLocked(p_dec, NULL, true);
> vlc/modules/codec/omxil/mediacodec.c:1620:        
> DecodeFlushLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1643: 
> AbortDecoderLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1656: 
> QueueBlockLocked(p_dec, NULL, true);
> vlc/modules/codec/omxil/mediacodec.c:1657:        
> DecodeFlushLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1675: 
> AbortDecoderLocked(p_dec);
> vlc/modules/codec/omxil/mediacodec.c:1683: 
> QueueBlockLocked(p_dec, p_in_block, false);
> vlc/src/input/decoder.c:267:static void DecoderUpdateFormatLocked( 
> struct decoder_owner *p_owner )
> vlc/src/input/decoder.c:383:        DecoderUpdateFormatLocked( p_owner 
> );
> vlc/src/input/decoder.c:581:    DecoderUpdateFormatLocked( p_owner );
> vlc/src/input/decoder.c:643:        DecoderUpdateFormatLocked( p_owner 
> );
> vlc/src/input/decoder.c:897:            DecoderUpdateFormatLocked( 
> p_owner );
> vlc/src/input/es_out.c:224:static void         EsOutDelLocked( es_out_t 
> *, es_out_id_t * );
> vlc/src/input/es_out.c:246:static int EsOutControlLocked( es_out_t 
> *out, 
> int i_query, ... );
> vlc/src/input/es_out.c:728:    EsOutControlLocked(out, 
> ES_OUT_SET_JITTER, p_sys->i_pts_delay,
> vlc/src/input/es_out.c:746:    EsOutControlLocked(out, 
> ES_OUT_SET_JITTER, p_sys->i_pts_delay,
> vlc/src/input/es_out.c:1871:static es_out_id_t *EsOutAddSlaveLocked( 
> es_out_t *out, const es_format_t *fmt,
> vlc/src/input/es_out.c:1989:    es_out_id_t *es = EsOutAddSlaveLocked( 
> out, fmt, NULL );
> vlc/src/input/es_out.c:2231:        EsOutDelLocked( out, 
> parent->cc.pp_es[i] );
> vlc/src/input/es_out.c:2480:        *pp_es = EsOutAddSlaveLocked( out, 
> &fmt, parent );
> vlc/src/input/es_out.c:2626:static void EsOutDelLocked( es_out_t *out, 
> es_out_id_t *es )
> vlc/src/input/es_out.c:2692:    EsOutDelLocked( out, es );
> vlc/src/input/es_out.c:2696:static int EsOutVaControlLocked( es_out_t 
> *, 
> int, va_list );
> vlc/src/input/es_out.c:2697:static int EsOutControlLocked( es_out_t 
> *out, int i_query, ... )
> vlc/src/input/es_out.c:2702:    int ret = EsOutVaControlLocked( out, 
> i_query, args );
> vlc/src/input/es_out.c:2745:static int EsOutVaControlLocked( es_out_t 
> *out, int i_query, va_list args )
> vlc/src/input/es_out.c:3109:                EsOutControlLocked( out, 
> ES_OUT_RESET_PCR );
> vlc/src/input/es_out.c:3111:                EsOutControlLocked( out, 
> ES_OUT_SET_JITTER,
> vlc/src/input/es_out.c:3267:        int i_ret = EsOutControlLocked( 
> out, 
> i_new_query, p_es );
> vlc/src/input/es_out.c:3557:    i_ret = EsOutVaControlLocked( out, 
> i_query, args );
> vlc/src/input/es_out_timeshift.c:258:static int          
> TsPopCmdLocked( 
> ts_thread_t *, ts_cmd_t *, bool b_flush );
> vlc/src/input/es_out_timeshift.c:599:static int ControlLocked( es_out_t 
> *p_out, int i_query, va_list args )
> vlc/src/input/es_out_timeshift.c:765:    i_ret = ControlLocked( p_out, 
> i_query, args );
> vlc/src/input/es_out_timeshift.c:853:        if( TsPopCmdLocked( p_ts, 
> &cmd, true ) )
> vlc/src/input/es_out_timeshift.c:900:static int TsPopCmdLocked( 
> ts_thread_t *p_ts, ts_cmd_t *p_cmd, bool b_flush )
> vlc/src/input/es_out_timeshift.c:1013:            if( ( !p_ts->b_paused 
> || b_buffering ) && !TsPopCmdLocked( p_ts, &cmd, false ) )
> vlc/src/input/input.c:1423:static size_t ControlGetReducedIndexLocked( 
> input_thread_t *p_input,
> vlc/src/input/input.c:1477:    size_t i_next_control_idx = 
> ControlGetReducedIndexLocked( p_input, &c );
> vlc/src/input/item.c:253:const char 
> *input_item_GetMetaLocked(input_item_t *item,
> vlc/src/input/item.c:267:    const char *value = 
> input_item_GetMetaLocked( p_i, meta_type );
> vlc/src/input/resource.c:327:static void 
> input_resource_PutVoutLocked(input_resource_t *p_resource,
> vlc/src/input/resource.c:363:    input_resource_PutVoutLocked( 
> p_resource, vout );
> vlc/src/input/resource.c:446: 
> input_resource_PutVoutLocked(p_resource, cfg->vout);
> vlc/src/media_source/media_tree.c:73:vlc_media_tree_AssertLocked(vlc_media_tree_t 
> *tree)
> vlc/src/media_source/media_tree.c:90: 
> vlc_media_tree_AssertLocked(tree); \
> vlc/src/media_source/media_tree.c:298:    
> vlc_media_tree_AssertLocked(tree);
> vlc/src/media_source/media_tree.c:314:    
> vlc_media_tree_AssertLocked(tree);
> vlc/src/media_source/media_tree.c:325:    
> vlc_media_tree_AssertLocked(tree);
> vlc/src/misc/background_worker.c:311:static void 
> BackgroundWorkerCancelLocked(struct background_worker *worker,
> vlc/src/misc/background_worker.c:332: 
> BackgroundWorkerCancelLocked(worker, id);
> vlc/src/misc/background_worker.c:355: 
> BackgroundWorkerCancelLocked(worker, NULL);
> vlc/src/playlist/content.c:184:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:191:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:198:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:208:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:220:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:232:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:245:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:268:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:295:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:308:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:330:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/content.c:356:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:114:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:121:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:129:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:142:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:154:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:257:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:272:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:287:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:302:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:317:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:337:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:344:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:351:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:379:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:407:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:427:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/control.c:439:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/export.c:58:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/notify.c:53:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/notify.c:75:    vlc_playlist_AssertLocked(playlist); 
> VLC_UNUSED(playlist);
> vlc/src/playlist/notify.c:115:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/player.c:42:    vlc_playlist_AssertLocked(playlist);
> //~ vlc/src/playlist/player.c:83:    
> vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/player.c:97:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/player.c:144:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/preparse.c:40:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/preparse.c:54:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/preparse.c:69:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/request.c:34:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/request.c:209:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/request.c:239:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/request.c:263:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/shuffle.c:35:    vlc_playlist_AssertLocked(playlist);
> vlc/src/playlist/sort.c:79:            const char *value = 
> input_item_GetMetaLocked(media, vlc_meta_Title);
> vlc/src/playlist/sort.c:96:            const char *value = 
> input_item_GetMetaLocked(media,
> vlc/src/playlist/sort.c:102:            const char *value = 
> input_item_GetMetaLocked(media, vlc_meta_Album);
> vlc/src/playlist/sort.c:107:            const char *value = 
> input_item_GetMetaLocked(media,
> vlc/src/playlist/sort.c:114:            const char *value = 
> input_item_GetMetaLocked(media, vlc_meta_Genre);
> vlc/src/playlist/sort.c:119:            const char *str = 
> input_item_GetMetaLocked(media, vlc_meta_Date);
> vlc/src/playlist/sort.c:127:            const char *str = 
> input_item_GetMetaLocked(media,
> vlc/src/playlist/sort.c:136:            const char *str = 
> input_item_GetMetaLocked(media,
> vlc/src/playlist/sort.c:145:            const char *value = 
> input_item_GetMetaLocked(media, vlc_meta_URL);
> vlc/src/playlist/sort.c:150:            const char *str = 
> input_item_GetMetaLocked(media, vlc_meta_Rating);
> vlc/src/playlist/sort.c:368:    vlc_playlist_AssertLocked(playlist);
> vlc/src/video_output/video_output.c:200:static void 
> vout_UpdateWindowSizeLocked(vout_thread_t *vout)
> vlc/src/video_output/video_output.c:407: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:474: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:494: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:517: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:549: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:581: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:2001:static int 
> EnableWindowLocked(vout_thread_t *vout, const video_format_t *original)
> vlc/src/video_output/video_output.c:2029: 
> vout_UpdateWindowSizeLocked(vout);
> vlc/src/video_output/video_output.c:2055:    if 
> (EnableWindowLocked(vout, &original) != 0)
> vlc/src/video_output/video_output.c:2109: 
> EnableWindowLocked(cfg->vout, &original);
> 
> On 2020-01-13 13:58, Rémi Denis-Courmont wrote:
> > A function that does not take necessary locks is Unlocked, e.g. fputc vs 
> > fputc_unlocked. The names in this patch are misleading.
> > 
> > Le 13 janvier 2020 13:07:31 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a 
> > écrit :
> > 
> >     ------------------------------------------------------------------------
> >       src/video_output/video_output.c | 15 +++++++++------
> >       1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> >     diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> >     index 659ede546ba..53c6416a620 100644
> >     --- a/src/video_output/video_output.c
> >     +++ b/src/video_output/video_output.c
> >     @@ -1998,7 +1998,7 @@ int vout_ChangeSource( vout_thread_t *vout, const video_format_t *original )
> >           return -1;
> >       }
> >       
> >     -static int vout_EnableWindow(vout_thread_t *vout, const video_format_t *original,
> >     +static int EnableWindowLocked(vout_thread_t *vout, const video_format_t *original,
> >                                    vlc_decoder_device **pp_dec_device)
> >       {
> >           vout_thread_sys_t *sys = vout->p;
> >     @@ -2006,7 +2006,6 @@ static int vout_EnableWindow(vout_thread_t *vout, const video_format_t *original
> >           assert(!sys->dummy);
> >           assert(vout != NULL);
> >       
> >     -    vlc_mutex_lock(&sys->window_lock);
> >           if (!sys->window_enabled) {
> >               vout_window_cfg_t wcfg = {
> >                   .is_fullscreen = var_GetBool(vout, "fullscreen"),
> >     @@ -2022,7 +2021,6 @@ static int vout_EnableWindow(vout_thread_t *vout, const video_format_t *original
> >               vout_SizeWindow(vout, original, &wcfg.width, &wcfg.height);
> >       
> >               if (vout_window_Enable(sys->display_cfg.window, &wcfg)) {
> >     -            vlc_mutex_unlock(&sys->window_lock);
> >                   msg_Err(vout, "failed to enable window");
> >                   return -1;
> >               }
> >     @@ -2036,7 +2034,6 @@ static int vout_EnableWindow(vout_thread_t *vout, const video_format_t *original
> >                   sys->dec_device = vlc_decoder_device_Create(&vout->obj, sys->display_cfg.window);
> >               *pp_dec_device = sys->dec_device ? vlc_decoder_device_Hold( sys->dec_device ) : NULL;
> >           }
> >     -    vlc_mutex_unlock(&sys->window_lock);
> >           return 0;
> >       }
> >       
> >     @@ -2061,13 +2058,16 @@ int vout_Request(const vout_configuration_t *cfg, vlc_video_context *vctx, input
> >               return 0;
> >           }
> >       
> >     -    if (vout_EnableWindow(vout, &original, NULL) != 0)
> >     +    vlc_mutex_lock(&sys->window_lock);
> >     +    if (EnableWindowLocked(vout, &original, NULL) != 0)
> >           {
> >               /* the window was not enabled, nor the display started */
> >               msg_Err(vout, "failed to enable window");
> >               video_format_Clean(&original);
> >     +        vlc_mutex_unlock(&sys->window_lock);
> >               return -1;
> >           }
> >     +    vlc_mutex_unlock(&sys->window_lock);
> >       
> >           if (sys->display != NULL)
> >               vout_StopDisplay(vout);
> >     @@ -2105,6 +2105,7 @@ vlc_decoder_device *vout_GetDevice(const vout_device_configuration_t *cfg)
> >           vlc_decoder_device *dec_device = NULL;
> >       
> >           assert(cfg->fmt != NULL);
> >     +    vout_thread_sys_t *sys = cfg->vout->p;
> >       
> >           if (!VoutCheckFormat(cfg->fmt))
> >               return NULL;
> >     @@ -2112,7 +2113,9 @@ vlc_decoder_device *vout_GetDevice(const vout_device_configuration_t *cfg)
> >           video_format_t original;
> >           VoutFixFormat(&original, cfg->fmt);
> >       
> >     -    int res = vout_EnableWindow(cfg->vout, &original, &dec_device);
> >     +    vlc_mutex_lock(&sys->window_lock);
> >     +    int res = EnableWindowLocked(cfg->vout, &original, &dec_device);
> >     +    vlc_mutex_unlock(&sys->window_lock);
> >           video_format_Clean(&original);
> >           if (res != 0)
> >               return NULL;
> > 
> > 
> > -- 
> > Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser 
> > ma brièveté.
> > 
> > _______________________________________________
> > 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