[FFmpeg-devel] [PATCH] PCX encoder
Michael Niedermayer
michaelni
Tue Mar 17 00:05:08 CET 2009
On Sun, Mar 15, 2009 at 08:28:32PM -0400, Daniel Verkamp wrote:
> On Sun, Mar 15, 2009 at 7:40 PM, Daniel Verkamp <daniel at drv.nu> wrote:
> > On Sun, Mar 15, 2009 at 7:38 PM, Daniel Verkamp <daniel at drv.nu> wrote:
> >> 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
> >>
> >
> > And of course it would help if I attached the patch...
> >
>
> This time without \r\n line endings...
[...]
> +/**
> + * 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++) {
^^^^^^^^^^^^^
useless
> + 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;
> + } else {
> + dst_used += 2;
> + if (dst_used > dst_size)
> + return -1;
> + *dst++ = 0xC0 | count;
> + }
i would put a dst_size < 2LL*src_size outside
[...]
> + filler = 128 - (buf - buf_start);
> + memset(buf, 0, filler);
> + buf += filler;
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");
> + return -1;
memleak
> + }
> + buf += written;
> + }
> + src += pict->linesize[0];
> + }
> +
> + av_free(plane);
> +
> + if (nplanes == 1 && bpp == 8) {
> + if (buf + 257 > buf_end) {
this has a small chance of overflow
if(buf_end - buf < 257)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20090317/189c992a/attachment.pgp>
More information about the ffmpeg-devel
mailing list