Re: [FFmpeg-devel] [PATCH] Add DXV encoder with support for DXT1 texture format

[Vittorio Giovara <vittorio.giovara@gmail.com>]

Hi,
thanks for the patch, below a few minor nits

On Wed, Jan 17, 2024 at 9:57 PM Connor Worley <connorbworley@gmail.com>
wrote:

> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 93ce8e3224..ef8c3a6d7d 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -106,6 +106,7 @@ extern const FFCodec ff_dvvideo_encoder;
>  extern const FFCodec ff_dvvideo_decoder;
>  extern const FFCodec ff_dxa_decoder;
>  extern const FFCodec ff_dxtory_decoder;
> +extern const FFCodec ff_dxv_encoder;
>  extern const FFCodec ff_dxv_decoder;
>

nit: keep list in order


>  extern const FFCodec ff_eacmv_decoder;
>  extern const FFCodec ff_eamad_decoder;
> diff --git a/libavcodec/dxvenc.c b/libavcodec/dxvenc.c
> new file mode 100644
> index 0000000000..33080fa1c9
> --- /dev/null
> +++ b/libavcodec/dxvenc.c
> @@ -0,0 +1,358 @@
> +/*
> + * Resolume DXV encoder
> + * Copyright (C) 2015 Vittorio Giovara <vittorio.giovara@gmail.com>
> + * Copyright (C) 2015 Tom Butterworth <bangnoise@gmail.com>
> + * Copyright (C) 2018 Paul B Mahol
> + * Copyright (C) 2024 Connor Worley <connorbworley@gmail.com>
>

Idk about tom or paul, but I haven't done anything for this encoder :)
I think you can prune the list of copyright quite a bit here


> +#define LOOKBACK_HT_ELEMS 0x40000
>

What does the HT stand for?


> +#define LOOKBACK_WORDS    0x20202
> +
> +enum DXVTextureFormat {
> +    DXV_FMT_DXT1 = MKBETAG('D', 'X', 'T', '1'),
> +};
>

Why would you go for an enum here? Just for future expansion and the switch
case below?

lgtm overall
-- 
Vittorio
_______________________________________________
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".