On Fri, Jul 08, 2011 at 10:25:48AM +0100, Joseph Artsimovich wrote:
>
> The renaming patch is to be applied first, followed by the main one.
> Both patches apply cleanly to the current libav Git head.
Could you resend them via git-send-email or at least create them
with with git-format-patch? That preserves author information
automatically and is easier for us to handle.
> --- a/libavcodec/dnxhddata.c
> +++ b/libavcodec/dnxhddata.c
> @@ -286,6 +330,37 @@ static const uint8_t dnxhd_1235_1241_run[62] = {
>
> +static const uint16_t dnxhd_1250_ac_codes[257] = {
Please format these tables.
> --- a/libavcodec/dnxhddec.c
> +++ b/libavcodec/dnxhddec.c
> @@ -43,20 +47,33 @@ typedef struct {
>
> +static void dnxhd_8bit_idct_put(DNXHDContext* ctx, uint8_t * dest/*align
> 16*/, int line_size, DCTELEM *block/*align 16*/)
Please break lines after 80 characters where easily possible.
> @@ -110,8 +127,24 @@ static int dnxhd_decode_header(DNXHDContext *ctx, const
> uint8_t *buf, int buf_si
>
> if (buf[0x21] & 0x40) {
> - av_log(ctx->avctx, AV_LOG_ERROR, "10 bit per component\n");
> - return -1;
> + ctx->avctx->pix_fmt = PIX_FMT_YUV422P16;
> + if (ctx->initialized_for_bits != 10) {
> + // We actually call ff_faanidct10_put() directly,
> + // but we still need a compatible permutation table.
nit: /* */ are generally preferred for multiline comments.
> --- a/libavcodec/dnxhdenc.c
> +++ b/libavcodec/dnxhdenc.c
> @@ -27,10 +29,13 @@
> #include "libavutil/opt.h"
> #include "avcodec.h"
> #include "dsputil.h"
> +#include "faandct.h"
> #include "mpegvideo.h"
> #include "dnxhdenc.h"
> +#include <assert.h>
Place system headers before local headers please, extra good karma for
a separating empty line.
> @@ -39,6 +44,8 @@ static const AVOption options[]={
> static const AVClass class = { "dnxhd", av_default_item_name, options,
> LIBAVUTIL_VERSION_INT };
>
> int dct_quantize_c(MpegEncContext *s, DCTELEM *block, int n, int qscale, int
> *overflow);
> +static void dnxhd_8bit_get_blocks(DNXHDEncContext *ctx, int mb_x, int mb_y);
> +static void dnxhd_10bit_get_blocks(DNXHDEncContext *ctx, int mb_x, int mb_y);
Please avoid forward declarations. These two should not even be necessary.
> @@ -59,6 +66,69 @@ static av_always_inline void dnxhd_get_pixels_8x4(DCTELEM
> *restrict block, const
>
> + memcpy(block , block- 8, sizeof(*block)*8);
> + memcpy(block+ 8, block-16, sizeof(*block)*8);
> + memcpy(block+16, block-24, sizeof(*block)*8);
> + memcpy(block+24, block-32, sizeof(*block)*8);
A few spaces around operators could make this more readable IMO.
> + for (int i = 1; i < 64; ++i) {
> + int j = scantable[i];
> + int unquantized = block[j];
> + int quantized = abs(unquantized * qmat[j]) >> DNX10BIT_QMAT_SHIFT;
> + if (unquantized < 0) {
> + quantized = -quantized;
> + }
> +
> + block[j] = (DCTELEM)quantized;
> +
> + if (quantized) {
> + last_non_zero = i;
> + }
nit: We generally drop unnecessary {} around if/for blocks.
> @@ -119,31 +189,55 @@ static int dnxhd_init_qmat(DNXHDEncContext *ctx, int
> lbias, int cbias)
> // init first elem to 1 to avoid div by 0 in convert_matrix
> uint16_t weight_matrix[64] = {1,}; // convert_matrix needs uint16_t*
> int qscale, i;
> + const uint8_t* luma_weight_table = ctx->cid_table->luma_weight;
> + const uint8_t* chroma_weight_table = ctx->cid_table->chroma_weight;
nit: vertically align the '='
> @@ -166,10 +260,22 @@ static int dnxhd_init_rc(DNXHDEncContext *ctx)
> static int dnxhd_encode_init(AVCodecContext *avctx)
> {
> DNXHDEncContext *ctx = avctx->priv_data;
> - int i, index;
> + int i, index, bit_depth;
> +
> + switch (avctx->pix_fmt) {
> + case PIX_FMT_YUV422P:
> + bit_depth = 8;
> + break;
> + case PIX_FMT_YUV422P16:
> + bit_depth = 10;
> + break;
> + default:
> + av_log(avctx, AV_LOG_ERROR, "pixel format is incompatible with
> DNxHD\n");
> + return -1;
> + }
Please indent the case at the same label as the switch and then indent
the code inside the case blocks.
> @@ -183,6 +289,21 @@ static int dnxhd_encode_init(AVCodecContext *avctx)
> + // FF_IDCT_FAAN is the only 10-bit safe IDCT.
> + // IDCT is used if encoding with FF_MB_DECISION_RD.
> + avctx->idct_algo = FF_IDCT_FAAN;
> +
> + ctx->dct_quantize = dnxhd_10bit_dct_quantize;
> + ctx->get_blocks = &dnxhd_10bit_get_blocks;
> + } else {
> + ctx->dct_quantize = dnxhd_8bit_dct_quantize;
> + ctx->get_blocks = &dnxhd_8bit_get_blocks;
> + }
nit: vertically align the '='
more below
> @@ -416,7 +582,7 @@ static int dnxhd_calc_bits_thread(AVCodecContext *avctx,
> void *arg, int jobnr, i
>
> ctx->m.last_dc[0] =
> ctx->m.last_dc[1] =
> - ctx->m.last_dc[2] = 1024;
> + ctx->m.last_dc[2] = 1<<(ctx->cid_table->bit_depth+2);
nit: space around operators
> @@ -464,21 +632,20 @@ static int dnxhd_encode_thread(AVCodecContext *avctx,
> void *arg, int jobnr, int
>
> ctx->m.last_dc[0] =
> ctx->m.last_dc[1] =
> - ctx->m.last_dc[2] = 1024;
> + ctx->m.last_dc[2] = 1<<(ctx->cid_table->bit_depth+2);
ditto
> @@ -514,13 +681,39 @@ static int dnxhd_mb_var_thread(AVCodecContext *avctx,
> void *arg, int jobnr, int
> + } else { // 10-bit
> + int const linesize = (ctx->m.linesize >> 1);
pointless ()
> + for (mb_x = 0; mb_x < ctx->m.mb_width; ++mb_x) {
> + uint16_t *pix = (uint16_t*)ctx->thread[0]->src[0] + ((mb_y<<4) *
> linesize) + (mb_x << 4);
nit: space around <<
> + for (int i = 0; i < 16; ++i) {
> + for (int j = 0; j < 16; ++j) {
> + // Turn 16-bit pixels into 10-bit ones.
> + int const sample = (unsigned)pix[j] >> 6;
That cast is suspicious, it seems useless to me.
> --- a/libavcodec/faandct.h
> +++ b/libavcodec/faandct.h
> @@ -36,4 +36,16 @@
>
> +/**
> + * @brief Does the DCT, producing coefficients multiplied by 4.
> + *
> + * Multiplying by 4 rather than 8 is done to avoid overflowing
> + * on 10 bit samples. This version can work on less than 10 bit
> + * samples as well.
> + *
> + * @note FAAN_POSTSCALE doesn't have any effect on this function.
> + * @note The caller is responsible for calling emms_c() if necessary.
> + */
> +void ff_faandct_10bit_safe(DCTELEM * data);
Please also document the parameter.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
