[vlc-devel] [PATCH v2 01/24] picture: add helpers for picture chaining

[Steve Lhomme]

On 2020-09-19 16:34, Rémi Denis-Courmont wrote:
> Le perjantaina 18. syyskuuta 2020, 17.45.07 EEST Steve Lhomme a écrit :
>> Picture chains are used either to keep a list of picture (in FIFO order) or
>> to attach a picture to another and return them all at once (filters). ---
>>   include/vlc_picture.h | 90 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/include/vlc_picture.h b/include/vlc_picture.h
>> index 7bb9ee5083c..44fcf6f3cec 100644
>> --- a/include/vlc_picture.h
>> +++ b/include/vlc_picture.h
>> @@ -167,6 +167,96 @@ static inline vlc_video_context*
>> picture_GetVideoContext(picture_t *pic) return pic->context ?
>> pic->context->vctx : NULL;
>>   }
>>
>> +/**
>> + * picture chaining helpers
>> + */
>> +
>> +/**
>> + * Check whether a picture has other pictures linked
>> + */
>> +static inline bool picture_has_chained_pics(const picture_t *pic)
>> +{
>> +    return chain->p_next != NULL;
>> +}
> 
> TBH, I have mixed feelings about the style.
> 
> In principles, I prefer lower-case and underscore. But in VLC, the habit is to
> capitalise the verb - unless the function is named after some non-VLC
> function.

OK (and vlc_ prefix as well).

>> +
>> +/**
>> + * Pop the chain from a picture chain.
> 
> ? Do you mean pop a picture from the picture  chain?

Nope, it does what it says. It's ugly/dirty but it's used in snapshots. 
Which eventually leak the chain because it thinks it only has a picture. 
For now I'm just doing an API around the existing use of 
picture::p_next, but it helps spotting such errors.

>> + *
>> + * The next picture in the chain becomes the front of the picture chain.
>> + *
>> + * \return the front of the picture chain, with other pictures still
>> chained
>> + */
>> +static inline picture_t * picture_chain_pop_chain(picture_t **chain)
>> +{
>> +    picture_t *front = *chain;
>> +    if (front)
>> +        *chain = front->p_next;
>> +    return front;
>> +}
>> +
>> +/**
>> + * Pop the front of a picture chain.
>> + *
>> + * The next picture in the chain becomes the front of the picture chain.
>> + *
>> + * \return the front of the picture chain (the picture itself)
>> + */
>> +static inline picture_t * picture_chain_pop_front(picture_t **chain)
>> +{
>> +    picture_t *front = picture_chain_pop_chain(chain);
>> +    if (front)
>> +        // unlink the front picture from the rest of the chain
>> +        front->p_next = NULL;
>> +    return front;
>> +}
> 
> Why do you split those two functions? I don't think the 'next = NULL'
> assignment is worth optimising against.

The other function should probably not exist at all, since you end up 
with the chained pictures in 2 different chains. It should go away when 
snapshot it fixed.

I'm fine having picture_chain_pop_front() doing things on its own and 
keeping it that way when that happens.