[FFmpeg-devel] [PATCH] PCX encoder

[Michael Niedermayer]

On Thu, Mar 19, 2009 at 07:41:55PM -0400, Daniel Verkamp wrote:
> On Thu, Mar 19, 2009 at 3:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Thu, Mar 19, 2009 at 12:33:41PM -0400, Daniel Verkamp wrote:
> >> On Wed, Mar 18, 2009 at 9:23 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > On Tue, Mar 17, 2009 at 01:14:50PM -0400, Daniel Verkamp wrote:
> >> > [...]
> >> >> +static int pcx_rle_encode( ? ? ?uint8_t *dst, int dst_size,
> >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ?const uint8_t *src, int src_size)
> >> >> +{
> >> >> + ? ?int i;
> >> >> + ? ?uint8_t prev = src[0];
> >> >> + ? ?int count = 1;
> >> >> + ? ?const uint8_t *dst_start = dst;
> >> >> +
> >> >> + ? ?// check worst-case upper bound on dst_size
> >> >> + ? ?if (dst_size < 2LL * src_size)
> >> >> + ? ? ? ?return -1;
> >> >> +
> >> >> + ? ?for (i = 1; ; i++) {
> >> >> + ? ? ? ?if (i != src_size && src[i] == prev && count < 0x3F) {
> >> >> + ? ? ? ? ? ?// current byte is same as prev
> >> >> + ? ? ? ? ? ?++count;
> >> >> + ? ? ? ?} else {
> >> >> + ? ? ? ? ? ?// output prev * count
> >> >> + ? ? ? ? ? ?if (count != 1 || prev >= 0xC0)
> >> >> + ? ? ? ? ? ? ? ?*dst++ = 0xC0 | count;
> >> >> + ? ? ? ? ? ?*dst++ = prev;
> >> >> +
> >> >> + ? ? ? ? ? ?if (i == src_size)
> >> >> + ? ? ? ? ? ? ? ?return dst - dst_start;
> >> >> +
> >> >> + ? ? ? ? ? ?// start new run
> >> >> + ? ? ? ? ? ?count = 1;
> >> >> + ? ? ? ? ? ?prev = src[i];
> >> >> + ? ? ? ?}
> >> >> + ? ?}
> >> >> +
> >> >> + ? ?return 0; // gcc warns even though this cannot be reached
> >> >
> >> > does gcc still warn with the current for() ?
> >> >
> >>
> >> Eliminated in latest patch.
> >>
> >> >
> >> >> +}
> >> >> +
> >> >> +static int pcx_encode_frame(AVCodecContext *avctx,
> >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned char *buf, int buf_size, void *data)
> >> >> +{
> >> >> + ? ?PCXContext *s = avctx->priv_data;
> >> >> + ? ?AVFrame *const pict = &s->picture;
> >> >> + ? ?const uint8_t *buf_start = buf;
> >> >> + ? ?const uint8_t *buf_end ? = buf + buf_size;
> >> >> +
> >> >> + ? ?int bpp, nplanes, i, j, y, p, line_bytes, written, src_line_size;
> >> >> + ? ?const uint32_t *pal = NULL;
> >> >> + ? ?const uint8_t *src;
> >> >> + ? ?uint8_t *plane;
> >> >> +
> >> >> + ? ?*pict = *(AVFrame *)data;
> >> >> + ? ?pict->pict_type = FF_I_TYPE;
> >> >> + ? ?pict->key_frame = 1;
> >> >> +
> >> >> + ? ?if (avctx->width > 65535 || avctx->height > 65535) {
> >> >> + ? ? ? ?av_log(avctx, AV_LOG_ERROR, "image dimensions do not fit in 16 bits\n");
> >> >> + ? ? ? ?return -1;
> >> >> + ? ?}
> >> >> +
> >> >> + ? ?switch (avctx->pix_fmt) {
> >> >> + ? ?case PIX_FMT_RGB24:
> >> >> + ? ? ? ?bpp = 8;
> >> >> + ? ? ? ?nplanes = 3;
> >> >> + ? ? ? ?break;
> >> >> + ? ?case PIX_FMT_RGB8:
> >> >> + ? ?case PIX_FMT_BGR8:
> >> >> + ? ?case PIX_FMT_RGB4_BYTE:
> >> >> + ? ?case PIX_FMT_BGR4_BYTE:
> >> >> + ? ?case PIX_FMT_GRAY8:
> >> >> + ? ?case PIX_FMT_PAL8:
> >> >> + ? ? ? ?bpp = 8;
> >> >> + ? ? ? ?nplanes = 1;
> >> >> + ? ? ? ?pal = (uint32_t *)pict->data[1];
> >> >> + ? ? ? ?break;
> >> >> + ? ?case PIX_FMT_MONOBLACK:
> >> >> + ? ? ? ?bpp = 1;
> >> >> + ? ? ? ?nplanes = 1;
> >> >> + ? ? ? ?pal = monoblack_pal;
> >> >> + ? ? ? ?break;
> >> >> + ? ?default:
> >> >> + ? ? ? ?av_log(avctx, AV_LOG_ERROR, "unsupported pixfmt\n");
> >> >> + ? ? ? ?return -1;
> >> >> + ? ?}
> >> >> +
> >> >> + ? ?line_bytes = (avctx->width * bpp + 7) >> 3;
> >> >> + ? ?line_bytes = (line_bytes + 1) & ~1;
> >> >> +
> >> >> + ? ?bytestream_put_byte(&buf, 10); ? ? ? ? ? ? ? ? ?// manufacturer
> >> >> + ? ?bytestream_put_byte(&buf, 5); ? ? ? ? ? ? ? ? ? // version
> >> >> + ? ?bytestream_put_byte(&buf, 1); ? ? ? ? ? ? ? ? ? // encoding
> >> >> + ? ?bytestream_put_byte(&buf, bpp); ? ? ? ? ? ? ? ? // bits per pixel per plane
> >> >> + ? ?bytestream_put_le16(&buf, 0); ? ? ? ? ? ? ? ? ? // x min
> >> >> + ? ?bytestream_put_le16(&buf, 0); ? ? ? ? ? ? ? ? ? // y min
> >> >> + ? ?bytestream_put_le16(&buf, avctx->width - 1); ? ?// x max
> >> >> + ? ?bytestream_put_le16(&buf, avctx->height - 1); ? // y max
> >> >> + ? ?bytestream_put_le16(&buf, 0); ? ? ? ? ? ? ? ? ? // horizontal DPI
> >> >> + ? ?bytestream_put_le16(&buf, 0); ? ? ? ? ? ? ? ? ? // vertical DPI
> >> >> + ? ?for (i = 0; i < 16; i++)
> >> >> + ? ? ? ?bytestream_put_be24(&buf, pal ? pal[i] : 0);// palette (<= 16 color only)
> >> >> + ? ?bytestream_put_byte(&buf, 0); ? ? ? ? ? ? ? ? ? // reserved
> >> >> + ? ?bytestream_put_byte(&buf, nplanes); ? ? ? ? ? ? // number of planes
> >> >> + ? ?bytestream_put_le16(&buf, line_bytes); ? ? ? ? ?// scanline plane size in bytes
> >> >> +
> >> >> + ? ?while (buf - buf_start < 128)
> >> >> + ? ? ? ?*buf++= 0;
> >> >> +
> >> >> + ? ?plane = av_mallocz(line_bytes);
> >> >> + ? ?src = pict->data[0];
> >> >> + ? ?src_line_size = (avctx->width * nplanes * bpp + 7) >> 3;
> >> >> +
> >> >> + ? ?for (y = 0; y < avctx->height; y++) {
> >> >> + ? ? ? ?for (p = 0; p < nplanes; p++) {
> >> >> + ? ? ? ? ? ?for (i = p, j = 0; i < src_line_size; i += nplanes, j++) {
> >> >> + ? ? ? ? ? ? ? ?plane[j] = src[i];
> >> >> + ? ? ? ? ? ?}
> >> >> + ? ? ? ? ? ?if ((written = pcx_rle_encode(buf, buf_end - buf,
> >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?plane, line_bytes)) < 0) {
> >> >> + ? ? ? ? ? ? ? ?av_log(avctx, AV_LOG_ERROR, "buffer too small\n");
> >> >> + ? ? ? ? ? ? ? ?av_free(plane);
> >> >> + ? ? ? ? ? ? ? ?return -1;
> >> >> + ? ? ? ? ? ?}
> >> >
> >> > i think plane could be avoided by passing nplanes into pcx_rle_encode()
> >> > and i think that wouldnt make pcx_rle_encode uglier ...
> >> >
> >>
> >> Ok, reworked to move the plane handling into pcx_rle_encode().
> >
> > [...]
> >> +static int pcx_rle_encode( ? ? ?uint8_t *dst, int dst_size,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ?const uint8_t *src, int src_plane_size, int nplanes)
> >> +{
> >> + ? ?int p;
> >> + ? ?const uint8_t *dst_start = dst;
> >> +
> >> + ? ?// check worst-case upper bound on dst_size
> >> + ? ?if (dst_size < 2LL * src_plane_size * nplanes || src_plane_size <= 0)
> >> + ? ? ? ?return -1;
> >> +
> >> + ? ?for (p = 0; p < nplanes; p++) {
> >
> > i would have expected that to be outside the function but thats not
> > a request to change it, just a note
> >
> 
> Well, if support is ever added for 2 or 4 bits per pixel, the user
> won't be able to do something like "pass the address of the buffer +
> some bits" as the start of the plane to the encoder... it only works
> currently since the plane support is only used for bpp == 8 where
> picking out a value from a particular plane is just a matter of byte
> addressing, not the more complicated splitting that would be required
> for < 8 bpp.
> 
> In any case, I find it a little distasteful to put any of the plane
> stuff in the RLE encoder; it seems a little "impure" - but I guess it
> is worth it to eliminate the swizzling in an extra buffer (plus I
> doubt anything else uses PCX-style RLE encoding anyway, so this
> function probably won't get reused).
> 
> >
> >> + ? ? ? ?int i, count = 1;
> >> + ? ? ? ?const uint8_t *src_plane = src + p;
> >> + ? ? ? ?uint8_t prev = *src_plane;
> >> + ? ? ? ?src_plane += nplanes;
> >> +
> >> + ? ? ? ?for (i = 1; ; i++, src_plane += nplanes) {
> >> + ? ? ? ? ? ?if (i != src_plane_size && *src_plane == prev && count < 0x3F) {
> >> + ? ? ? ? ? ? ? ?// current byte is same as prev
> >> + ? ? ? ? ? ? ? ?++count;
> >> + ? ? ? ? ? ?} else {
> >> + ? ? ? ? ? ? ? ?// output prev * count
> >> + ? ? ? ? ? ? ? ?if (count != 1 || prev >= 0xC0)
> >> + ? ? ? ? ? ? ? ? ? ?*dst++ = 0xC0 | count;
> >> + ? ? ? ? ? ? ? ?*dst++ = prev;
> >> +
> >> + ? ? ? ? ? ? ? ?if (i == src_plane_size)
> >> + ? ? ? ? ? ? ? ? ? ?break;
> >> +
> >> + ? ? ? ? ? ? ? ?// start new run
> >> + ? ? ? ? ? ? ? ?count = 1;
> >> + ? ? ? ? ? ? ? ?prev = *src_plane;
> >> + ? ? ? ? ? ?}
> >> + ? ? ? ?}
> >
> > the number of variables in this loop can be reduced
> > a changed src_plane_size can be checked against src_plane thus making
> > i unneeded
> >
> 
> Reworked... is this what you had in mind?

yes patch ok

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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20090320/f7366619/attachment.pgp>