[FFmpeg-devel] [PATCH] DNxHD/VC-3 encoder

[Michael Niedermayer]

Hi

On Sat, Oct 06, 2007 at 02:10:52AM +0200, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> >>>
> >>> [...]
> >>>
> >>>>+    ctx->m.avctx = avctx;
> >>>>+    ctx->m.mb_intra = 1;
> >>>>+    ctx->m.h263_aic = 1;
> >>>>+
> >>>>+    MPV_common_init_mmx(&ctx->m);
> >>>>+    if (!ctx->m.dct_quantize)
> >>>>+        ctx->m.dct_quantize = dct_quantize_c;
> >>>
> >>>iam an idiot, i was staring at this for maybe 2 minutes, somehow i knew
> >>>it was wrong i dont understand why i didnt realize it immedeatly ...
> >>>
> >>>_mmx()
> >>>
> >>>you dont even know if the CPU is a x86 ...
> >>
> >>Added ifdef
> > 
> > 
> > NO!, a *codec *muxer has no business calling any *_mmx
> > think about it, what will happen with people on ppc, on arm, on sparc, ...
> > also grep, no other codec does that
> 
> Hummm, in DCT_common_init in mpegvideo.c:
> #if defined(HAVE_MMX)
>     MPV_common_init_mmx(s);
> #elif defined(ARCH_ALPHA)
>     MPV_common_init_axp(s);
> #elif defined(HAVE_MLIB)
>     MPV_common_init_mlib(s);
> #elif defined(HAVE_MMI)
>     MPV_common_init_mmi(s);
> #elif defined(ARCH_ARMV4L)
>     MPV_common_init_armv4l(s);
> #elif defined(HAVE_ALTIVEC)
>     MPV_common_init_altivec(s);
> #elif defined(ARCH_BFIN)
>     MPV_common_init_bfin(s);
> #endif
> 
> > find out which init function is the correct one and call it dont call
> > architecture specific functions directly
> > if theres no way to do what you want currently fix the code so it is
> > possible cleanly
> 
> Yes, but it seems Im doing exactly what is done in mpegvideo.c, no ?

that code is not specific to a single codec, or do you see it in msmpeg4.c
in h263.c, ... ?
also you copied only the _mmx (and if you would copy all it would be code
duplication)


> 
> >>> [...]
> >>>
> >>>>+static int dnxhd_encode_picture(AVCodecContext *avctx, unsigned char *buf, int buf_size, void *data)
> >>>>+{
> >>>>+    DNXHDEncContext *ctx = avctx->priv_data;
> >>>>+    AVFrame *input_frame = data;
> >>>>+    int first_field = 1;
> >>>>+    int offset, i, ret;
> >>>>+
> >>>>+    if (buf_size < ctx->cid_table->frame_size) {
> >>>>+        av_log(avctx, AV_LOG_ERROR, "output buffer is too small to compress picture\n");
> >>>>+        return -1;
> >>>>+    }
> >>>>+
> >>>>+    av_picture_copy((AVPicture *)ctx->picture, data, PIX_FMT_YUV422P, avctx->width, avctx->height);
> >>>
> >>>why do you copy the input image?
> >>
> >>If picture is not allocated as 1920x1088 I need to copy it.
> > 
> > 
> > why?
> 
> It segv in dsp->get_pixels, since last row height is not 16 but 8 (for
> example when encoding raw yuv422p stream).
> 
> I tried reading last row differently but:
> - it's not faster than copying and IMHO clutter the code.
> - for progressive its possible to skip 4 bottom blocks (8 lines) but
> interlaced mode is even more complicated since 4 lines are in field 1
> and 4 are in field 2.

so there are X lines at th bottom and you read more than X and that segfaults
and as "solution" you copy the whole 1920x1080 image
i really do think you should not read lines after the image

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071006/04d43d0e/attachment.pgp>