On Sun, Feb 12, 2012 at 05:01:26PM +0100, Diego Biurrun wrote:
> On Sun, Feb 12, 2012 at 04:49:26PM +0100, Kostya Shishkov wrote:
> > ---
> > Changelog | 1 +
> > doc/general.texi | 2 +-
> > libavcodec/Makefile | 1 +
> > libavcodec/allcodecs.c | 2 +-
> > libavcodec/proresdsp.c | 17 +
> > libavcodec/proresdsp.h | 3 +
> > libavcodec/proresenc.c | 820
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> > libavcodec/version.h | 2 +-
> > 8 files changed, 845 insertions(+), 3 deletions(-)
> > create mode 100644 libavcodec/proresenc.c
>
> \o/
>
> Do you plan to add encode/decode FATE tests later on?
Not sure it's a good idea yet. Encoder should get more stable first.
And someone should fix the bug in multithreaded decoding (which is triggered
by having different slice quantisers, like in my decoder output).
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -6,6 +6,7 @@ version <next>:
> > - Support for fragmentation in the mov/mp4 muxer
> > - ISMV (Smooth Streaming) muxer
> > +- ProRes encoder
>
> We write "Apple ProRes decoder" below, so you should match that here.
OK
> > --- /dev/null
> > +++ b/libavcodec/proresenc.c
> > @@ -0,0 +1,820 @@
> > +/*
> > + * Pear ProRes encoder
>
> :)
>
> Still I would suggest not pushing the joke into master.
OK
> > +#include "avcodec.h"
> > +#include "libavutil/opt.h"
> > +#include "put_bits.h"
> > +#include "bytestream.h"
> > +#include "proresdsp.h"
> > +#include "proresdata.h"
>
> nit: Move the libavutil include before the others.
done
> > + for (i = 0; i < ctx->num_planes; i++) {
> > + is_chroma[i] = (i == 1 || i == 2);
> > + plane_factor[i] = slice_width_factor + 2;
>
> nit: align
>
> > + if (!is_chroma[i] || ctx->chroma_factor == CFACTOR_Y444) {
> > + xp = x << 4;
> > + yp = y << 4;
> > + num_cblocks[i] = 4;
> > + pwidth = avctx->width;
> > + } else {
> > + xp = x << 3;
> > + yp = y << 4;
> > + num_cblocks[i] = 2;
> > + pwidth = avctx->width >> 1;
> > + }
>
> ditto
aligned
> > + *avctx->coded_frame = *pic;
> > + avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
> > + avctx->coded_frame->key_frame = 1;
>
> same
those too
> > + bytestream_put_be32 (&orig_buf, FRAME_ID); // frame container ID
> > + buf = orig_buf;
> > + bytestream_put_be16 (&buf, 0); // version 1
> > + bytestream_put_buffer(&buf, "Lavc", 4); // creator
> > + bytestream_put_be16 (&buf, avctx->width);
> > + bytestream_put_be16 (&buf, avctx->height);
> > + bytestream_put_byte (&buf, ctx->chroma_factor << 6); // frame flags
> > + bytestream_put_byte (&buf, 0); // reserved
> > + bytestream_put_byte (&buf, 0); // primaries
> > + bytestream_put_byte (&buf, 0); // transfer function
> > + bytestream_put_byte (&buf, 6); // colour matrix - ITU-R
> > BT.601-4
> > + bytestream_put_byte (&buf, 0x40); // source format and alpha
> > information
> > + bytestream_put_byte (&buf, 0); // reserved
> > + bytestream_put_byte (&buf, 0x03); // matrix flags - both
> > matrices are present
>
> I'd drop those spaces before (.
I'd rather not - it's vertically aligned that way.
> > + bytestream_put_be16 (&tmp, buf - orig_buf); // write back frame
> > header size
> > +
> > + // picture header
> > + picture_size_pos = buf + 1;
> > + bytestream_put_byte (&buf, 0x40); // picture header size (in
> > bits)
> > + buf += 4; // picture data size will
> > be stored here
> > + bytestream_put_be16 (&buf, ctx->num_slices); // total number of slices
> > + bytestream_put_byte (&buf, av_log2(ctx->mbs_per_slice) << 4); //
> > slice width and height in MBs
>
> same
and here it just continues
> > + av_log(avctx, AV_LOG_DEBUG, "profile %d, %d slices, %d bits per MB\n",
> > + ctx->profile, ctx->num_slices, ctx->bits_per_mb);
>
> av_dlog()?
No, it's an information that might be interesting to the user.
> > + for (i = min_quant; i <= max_quant; i++) {
> > + ctx->nodes[i].prev_node = -1;
> > + ctx->nodes[i].bits = 0;
> > + ctx->nodes[i].score = 0;
>
> align
done
New version will be sent with interface changed to encode2()
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
