Re: [FFmpeg-devel] [PATCH 1/7] lavc/flac_parser: use a custom FIFO implementation

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Anton Khirnov:
> FLAC parser currently uses AVFifoBuffer in a highly non-trivial manner,
> modifying its "internals" (the whole struct is currently public, but no
> other code touches its contents directly). E.g. it does not use any
> av_fifo functions for reading the FIFO contents, but implements its own.
> 
> Reimplement the needed parts of the AVFifoBuffer API in the FLAC parser,
> making it completely self-contained. This will allow us to make
> AVFifoBuffer private.
> ---
>  libavcodec/flac_parser.c | 191 +++++++++++++++++++++++++++++++--------
>  1 file changed, 153 insertions(+), 38 deletions(-)
> 
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index 3b27b152fc..d6af5ce836 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -34,7 +34,6 @@
>  
>  #include "libavutil/attributes.h"
>  #include "libavutil/crc.h"
> -#include "libavutil/fifo.h"
>  #include "bytestream.h"
>  #include "parser.h"
>  #include "flac.h"
> @@ -57,6 +56,15 @@
>  #define MAX_FRAME_HEADER_SIZE 16
>  #define MAX_FRAME_VERIFY_SIZE (MAX_FRAME_HEADER_SIZE + 1)
>  
> +typedef struct FifoBuffer {
> +    uint8_t *buffer;
> +    uint8_t *end;
> +    uint8_t *rptr;
> +    uint8_t *wptr;
> +    size_t rndx;
> +    size_t wndx;
> +} FifoBuffer;
> +
>  typedef struct FLACHeaderMarker {
>      int offset;       /**< byte offset from start of FLACParseContext->buffer */
>      int link_penalty[FLAC_MAX_SEQUENTIAL_HEADERS]; /**< array of local scores
> @@ -84,7 +92,7 @@ typedef struct FLACParseContext {
>      int nb_headers_buffered;       /**< number of headers that are buffered   */
>      int best_header_valid;         /**< flag set when the parser returns junk;
>                                          if set return best_header next time   */
> -    AVFifoBuffer *fifo_buf;        /**< buffer to store all data until headers
> +    FifoBuffer fifo_buf;           /**< buffer to store all data until headers
>                                          can be verified                       */
>      int end_padded;                /**< specifies if fifo_buf's end is padded */
>      uint8_t *wrap_buf;             /**< general fifo read buffer when wrapped */
> @@ -127,6 +135,17 @@ static int frame_header_is_valid(AVCodecContext *avctx, const uint8_t *buf,
>      return 1;
>  }
>  
> +static size_t flac_fifo_size(FifoBuffer *f)

Missing const

> +{
> +    av_assert0(f->wndx >= f->rndx);

This assert looks very fishy: It will trigger if wndx has already
wrapped around while rndx has not yet (or rather, if wndx has wrapped
around once more than rndx). Or am I missing something here?

> +    return f->wndx - f->rndx;
> +}
> +
> +static size_t flac_fifo_space(FifoBuffer *f)
> +{
> +    return f->end - f->buffer - flac_fifo_size(f);
> +}
> +

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".