[vlc-devel] [PATCH] decoder: remove misleading comment

[Steve Lhomme]

On 2019-09-03 14:09, Thomas Guillem wrote:
> 
> 
> On Tue, Sep 3, 2019, at 14:07, Thomas Guillem wrote:
>>
>>
>> On Tue, Sep 3, 2019, at 14:05, Steve Lhomme wrote:
>>> On 2019-09-03 13:43, Thomas Guillem wrote:
>>>>
>>>> On Tue, Sep 3, 2019, at 13:22, Steve Lhomme wrote:
>>>>> The p_aout and p_vout need locking for reading as well (it wouldn't make much
>>>>> sense to only lock when writing).
>>>>>
>>>>> For example p_aout can be modified in the DecoderThread during a ReloadDecoder.
>>>>
>>>> ReloadDecoder() will first unload the audio decoder before writing the aout so we are safe here. I could document it.
>>>
>>> It reads p_owner->p_aout and do aout_DecDelete( p_aout ) on it in the
>>> DecoderThread. It is the same kind of code done in the decoder thread
>>> calling aout_update_format(). It is safe only because the
>>> reading/resetting/setting is done under lock in both threads.
>>
>> No, it is safe before the module is unloaded, so no threads can call
>> aout_update_format().
> 
> s/before/because/

OK, I missed that part (decoder_Clean()).

>>
>>>
>>> IMO that contradicts the "These variables need locking on write(only)" doc.
>>>
>>>>>
>>>>> p_vout cannot be modified outside of the vout_update_format/spu_new_buffer but
>>>>> it is also access in the DecoderThread, for example during DecoderProcessFlush()
>>>>> or the OutputChangeXXX functions.
>>>>
>>>> That is what my comment say no ?
>>>
>>> Yes
>>>
>>>>>
>>>>> For functions only called by the decoder, it might be OK not to lock as the
>>>>> p_out/p_aout should only change during vout_update_format/aout_update_format
>>>>> which the decoder should call before doing other calls from various threads.
>>>>> ---
>>>>>    src/input/decoder.c | 3 +--
>>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/input/decoder.c b/src/input/decoder.c
>>>>> index 65e3dd6f6c..7011c99226 100644
>>>>> --- a/src/input/decoder.c
>>>>> +++ b/src/input/decoder.c
>>>>> @@ -101,12 +101,11 @@ struct decoder_owner
>>>>>        vlc_cond_t  wait_acknowledge;
>>>>>        vlc_cond_t  wait_fifo; /* TODO: merge with wait_acknowledge */
>>>>>    
>>>>> -    /* -- These variables need locking on write(only) -- */
>>>>> +    /* -- Theses variables need locking on read *and* write -- */
>>>>>        audio_output_t *p_aout;
>>>>>    
>>>>>        vout_thread_t   *p_vout;
>>>>>    
>>>>> -    /* -- Theses variables need locking on read *and* write -- */
>>>>>        /* Preroll */
>>>>>        vlc_tick_t i_preroll_end;
>>>>>    
>>>>> -- 
>>>>> 2.17.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
>>>>
>>> _______________________________________________
>>> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>