[FFmpeg-devel] [PATCH] PCX encoder

[Daniel Verkamp]

On Sun, Mar 15, 2009 at 4:18 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sat, Mar 14, 2009 at 04:09:54PM -0400, Daniel Verkamp wrote:
>> On Sat, Mar 14, 2009 at 3:50 PM, Daniel Verkamp <daniel at drv.nu> wrote:
>> > Hi,
>> >
>> > Attached is a patch for a PCX image encoder that handles 1-, 8-, and
>> > 24-bpp pixfmts. ?1 and 8 bpp tested with pbrush from Win3.11, 24 bpp
>> > tested with IrfanView; if anyone has a version of PC Paintbrush that
>> > supports 24 bpp, please test, but I am fairly sure it is correct.
>> >
>> > Thanks,
>> > -- Daniel Verkamp
>> >
>>
>> Silly last-minute change broke compilation - fixed patch attached.
> [...]
>> +/**
>> + * PCX run-length encoder
>> + * @param dst output buffer
>> + * @param dst_size size of output buffer
>> + * @param src input buffer
>> + * @param src_size size of input buffer
>> + * @return number of bytes written to dst or -1 on error
>> + */
>> +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;
>> + ? ?int dst_used = 0;
>> +
>> + ? ?for (i = 1; i <= src_size; 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) {
>> + ? ? ? ? ? ? ? ?if (++dst_used > dst_size)
>> + ? ? ? ? ? ? ? ? ? ?return -1;
>> + ? ? ? ? ? ? ? ?*dst++ = prev;
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?dst_used += 2;
>> + ? ? ? ? ? ? ? ?if (dst_used > dst_size)
>> + ? ? ? ? ? ? ? ? ? ?return -1;
>> + ? ? ? ? ? ? ? ?*dst++ = 0xC0 | count;
>> + ? ? ? ? ? ? ? ?*dst++ = prev;
>> + ? ? ? ? ? ?}
>
> *dst++ = prev can be factored out
>

Done.

>
>> +
>> + ? ? ? ? ? ?// get new prev
>> + ? ? ? ? ? ?if (i != src_size) {
>> + ? ? ? ? ? ? ? ?count = 1;
>> + ? ? ? ? ? ? ? ?prev = src[i];
>> + ? ? ? ? ? ?}
>
> if(i == src_size)
> ? ?return dst_used;
> before this seems to make more sense and would also make the
> check in the loop unneeded
>

Written as I think you mean - the check is still in the loop,
though...  The reason I put the check in the loop in the first place
is to avoid duplicating the output part (deciding whether the run can
be written in one byte or two) for the final byte.

>
> [...]
>
>> + ? ?for (i = 0; i < avctx->height; i++) {
>
> y is better than i for the vertical coordinate
>
>
>> + ? ? ? ?for (j = 0; j < nplanes; j++) {
>
> p is better than j for plane
>

Changed both of above variable names.

>
>> + ? ? ? ? ? ?for (k = j, l = 0; k < src_line_size; k += nplanes, l++) {
>> + ? ? ? ? ? ? ? ?plane[l] = src[k];
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?if ((written = pcx_rle_encode(buf, buf_size - (buf - bufstart),
>
> i think a buf_end pointer could simplify this
>

Indeed, reworked to use a buf_end.

>
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?plane, line_bytes)) < 0) {
>> + ? ? ? ? ? ? ? ?av_log(avctx, AV_LOG_ERROR, "buffer too small\n");
>> + ? ? ? ? ? ? ? ?return -1;
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?buf += written;
>> + ? ? ? ?}
>> + ? ? ? ?src += p->linesize[0];
>> + ? ?}
>> +
>> + ? ?av_free(plane);
>> +
>> + ? ?if (nplanes == 1 && bpp == 8) {
>> + ? ? ? ?bytestream_put_byte(&buf, 12);
>> + ? ? ? ?for (i = 0; i < 256; i++) {
>> + ? ? ? ? ? ?bytestream_put_be24(&buf, pal[i]);
>> + ? ? ? ?}
>> + ? ?}
>
> this seems to be missing checks for buf_size
>

Good point, added one now.
Do I need one for the (128-byte) header as well, or can I assume buf
is always at least some constant size?

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

Thanks,
-- Daniel Verkamp