[vlc-devel] [PATCH v4 08/24] picture_fifo: simplify the picture fifo tail handling

[Steve Lhomme]

On 2020-09-21 18:28, Rémi Denis-Courmont wrote:
> Le maanantaina 21. syyskuuta 2020, 14.48.06 EEST Steve Lhomme a écrit :
>> Rather than a pointer on the last picture pointer, we use either NULL (no
>> tail/chain empty) or a pointer to the last picture in the chain.
>>
>> Make sure that the pictures we queue don't have a p_next otherwise it would
>> screw the computation of the last. (the issue existed before this patch)
>> ---
>>   src/misc/picture_fifo.c | 29 ++++++++++++++++++-----------
>>   1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/misc/picture_fifo.c b/src/misc/picture_fifo.c
>> index f999ec1a3f6..46838669e21 100644
>> --- a/src/misc/picture_fifo.c
>> +++ b/src/misc/picture_fifo.c
>> @@ -39,30 +39,37 @@
>>   struct picture_fifo_t {
>>       vlc_mutex_t lock;
>>       picture_t   *first;
>> -    picture_t   **last_ptr;
>> +    picture_t   *tail;
> 
> The current code is a very common pattern for FIFOs, that fits very well with
> C. It avoids making a bogus special case of the empty list. And indeed, this
> patch actually increases code size. This does not look like an improvement to
> me.

It becomes a one line in [24/24]. I modify it so it matches the code 
it's replaced with. The original idea was to use the same pattern for 
each "local" picture FIFO (ie not use the picture FIFO API).

>>   };
>>
>>   static void PictureFifoReset(picture_fifo_t *fifo)
>>   {
>>       fifo->first    = NULL;
>> -    fifo->last_ptr = &fifo->first;
>> +    fifo->tail     = NULL;
>>   }
>>   static void PictureFifoPush(picture_fifo_t *fifo, picture_t *picture)
>>   {
>>       assert(!picture->p_next);
>> -    *fifo->last_ptr = picture;
>> -    fifo->last_ptr  = &picture->p_next;
>> +    if (fifo->first == NULL)
>> +    {
>> +        fifo->first = picture;
>> +        fifo->tail  = picture;
>> +    }
>> +    else
>> +    {
>> +        fifo->tail->p_next = picture;
>> +        fifo->tail =  picture;
>> +        picture->p_next = NULL; // we're appending a picture, not a chain
>> +    }
>>   }
>>   static picture_t *PictureFifoPop(picture_fifo_t *fifo)
>>   {
>> -    picture_t *picture = fifo->first;
>> +    if (fifo->first == NULL)
>> +        return NULL;
>>
>> -    if (picture) {
>> -        fifo->first = picture->p_next;
>> -        if (!fifo->first)
>> -            fifo->last_ptr = &fifo->first;
>> -        picture->p_next = NULL;
>> -    }
>> +    picture_t *picture = fifo->first;
>> +    fifo->first = picture->p_next;
>> +    picture->p_next = NULL;
>>       return picture;
>>   }
> 
> 
> -- 
> Реми Дёни-Курмон
> http://www.remlab.net/
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>