Looks OK in general.
On Fri, Feb 09, 2018 at 10:51:36AM +0100, Luca Barbato wrote:
> --- /dev/null
> +++ b/libavcodec/libaom.c
> @@ -0,0 +1,90 @@
> +
> +#define HIGH_DEPTH(fmt) \
> + case AOM_IMG_FMT_I ## fmt ## 16: switch (depth) { \
> + case 10: return AV_PIX_FMT_YUV ## fmt ## P10; \
> + case 12: return AV_PIX_FMT_YUV ## fmt ## P12; \
> + default: return AV_PIX_FMT_NONE; \
Move the switch statement to the next line for better readability please.
> +enum AVPixelFormat ff_aom_imgfmt_to_pixfmt(aom_img_fmt_t img, int depth)
> +{
> + switch (img) {
> + case AOM_IMG_FMT_RGB24: return AV_PIX_FMT_RGB24;
> + case AOM_IMG_FMT_RGB565: return AV_PIX_FMT_RGB565BE;
> + case AOM_IMG_FMT_RGB555: return AV_PIX_FMT_RGB555BE;
> + case AOM_IMG_FMT_UYVY: return AV_PIX_FMT_UYVY422;
> + case AOM_IMG_FMT_YUY2: return AV_PIX_FMT_YUYV422;
> + case AOM_IMG_FMT_YVYU: return AV_PIX_FMT_YVYU422;
> + case AOM_IMG_FMT_BGR24: return AV_PIX_FMT_BGR24;
> + case AOM_IMG_FMT_ARGB: return AV_PIX_FMT_ARGB;
> + case AOM_IMG_FMT_ARGB_LE: return AV_PIX_FMT_BGRA;
> + case AOM_IMG_FMT_RGB565_LE: return AV_PIX_FMT_RGB565LE;
> + case AOM_IMG_FMT_RGB555_LE: return AV_PIX_FMT_RGB555LE;
> + case AOM_IMG_FMT_I420: return AV_PIX_FMT_YUV420P;
> + case AOM_IMG_FMT_I422: return AV_PIX_FMT_YUV422P;
> + case AOM_IMG_FMT_I444: return AV_PIX_FMT_YUV444P;
> + case AOM_IMG_FMT_444A: return AV_PIX_FMT_YUVA444P;
> + case AOM_IMG_FMT_I440: return AV_PIX_FMT_YUV440P;
I'd break those lines.
> +/* case AOM_IMG_FMT_I42016: return AV_PIX_FMT_YUV420P16;
> + case AOM_IMG_FMT_I42216: return AV_PIX_FMT_YUV422P16;
> + case AOM_IMG_FMT_I44416: return AV_PIX_FMT_YUV444P16; */
Why is this commented out?
> +aom_img_fmt_t ff_aom_pixfmt_to_imgfmt(enum AVPixelFormat pix)
> +{
> + switch (pix) {
> + case AV_PIX_FMT_RGB24: return AOM_IMG_FMT_RGB24;
> + case AV_PIX_FMT_RGB565BE: return AOM_IMG_FMT_RGB565;
> + case AV_PIX_FMT_RGB555BE: return AOM_IMG_FMT_RGB555;
> + case AV_PIX_FMT_UYVY422: return AOM_IMG_FMT_UYVY;
> + case AV_PIX_FMT_YUYV422: return AOM_IMG_FMT_YUY2;
> + case AV_PIX_FMT_YVYU422: return AOM_IMG_FMT_YVYU;
> + case AV_PIX_FMT_BGR24: return AOM_IMG_FMT_BGR24;
> + case AV_PIX_FMT_ARGB: return AOM_IMG_FMT_ARGB;
> + case AV_PIX_FMT_BGRA: return AOM_IMG_FMT_ARGB_LE;
> + case AV_PIX_FMT_RGB565LE: return AOM_IMG_FMT_RGB565_LE;
> + case AV_PIX_FMT_RGB555LE: return AOM_IMG_FMT_RGB555_LE;
> + case AV_PIX_FMT_YUV420P: return AOM_IMG_FMT_I420;
> + case AV_PIX_FMT_YUV422P: return AOM_IMG_FMT_I422;
> + case AV_PIX_FMT_YUV444P: return AOM_IMG_FMT_I444;
> + case AV_PIX_FMT_YUVA444P: return AOM_IMG_FMT_444A;
> + case AV_PIX_FMT_YUV440P: return AOM_IMG_FMT_I440;
> + case AV_PIX_FMT_YUV420P10: return AOM_IMG_FMT_I42016;
> + case AV_PIX_FMT_YUV422P10: return AOM_IMG_FMT_I42216;
> + case AV_PIX_FMT_YUV444P10: return AOM_IMG_FMT_I44416;
> + case AV_PIX_FMT_YUV420P12: return AOM_IMG_FMT_I42016;
> + case AV_PIX_FMT_YUV422P12: return AOM_IMG_FMT_I42216;
> + case AV_PIX_FMT_YUV444P12: return AOM_IMG_FMT_I44416;
same
> --- /dev/null
> +++ b/libavcodec/libaom.h
> @@ -0,0 +1,31 @@
> +
> +#ifndef AVCODEC_LIBAOM_H
> +#define AVCODEC_LIBAOM_H
> +
> +#include <aom/aom_codec.h>
> +
> +#include "libavutil/pixfmt.h"
> +
> +enum AVPixelFormat ff_aom_imgfmt_to_pixfmt(aom_img_fmt_t img, int depth);
> +aom_img_fmt_t ff_aom_pixfmt_to_imgfmt(enum AVPixelFormat pix);
> +
> +#endif /* AVCODEC_LIBAOM_H */
Is encoding support planned? Otherwise splitting this into several files
is kind of silly.
> --- /dev/null
> +++ b/libavcodec/libaomdec.c
> @@ -0,0 +1,137 @@
> +static av_cold int aom_init(AVCodecContext *avctx,
> + const struct aom_codec_iface *iface)
> +{
> + if (aom_codec_dec_init(&ctx->decoder, iface, &deccfg, 0) !=
> AOM_CODEC_OK) {
> + const char *error = aom_codec_error(&ctx->decoder);
> + av_log(avctx, AV_LOG_ERROR, "Failed to initialize decoder: %s\n",
> + error);
> + return AVERROR(EINVAL);
These don't look like user-supplied values, so I think EINVAL is not the
right error code.
> +static int aom_decode(AVCodecContext *avctx, void *data, int *got_frame,
> + AVPacket *avpkt)
> +{
> + *got_frame = 1;
odd spacing
> +static av_cold int aom_free(AVCodecContext *avctx)
Why not aom_close()?
> +AVCodec ff_libaom_av1_decoder = {
> + .init = av1_init,
> + .close = aom_free,
> + .decode = aom_decode,
Why av1_init and not aom_init?
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
