[FFmpeg-devel] [PATCH] [RFC] Second try at pixdesc.h:write_line()

Michael Niedermayer michaelni
Sun Apr 19 17:44:02 CEST 2009


On Sun, Apr 19, 2009 at 05:20:57PM +0200, Stefano Sabatini wrote:
> On date Sunday 2009-04-19 15:56:10 +0200, Michael Niedermayer encoded:
> > On Sun, Apr 19, 2009 at 12:35:44PM +0200, Stefano Sabatini wrote:
> > > Mmh what about this?
> > > 
> > >         while (w--) {
> > >             *p |= *src++ << (8 - depth + shift);
> > >             shift -= step;
> > >             p -= shift >> 3;
> > >             shift &= 7;
> > >         }
> > > 
> > > I'm not still sure it is what you meant.  Also AFAIU this can only
> > > work for a pixel depth of 1, while pixel/source can contain pixels
> > > with a depth up to 16 bits (uint16_t).
> > > 
> > > Note that I cannot shift for a negative value (result is undefined).
> > 
> > i meant what i wrote
> > *p |= *src++ << shift;
> > 
> > and 
> > shift &= 7; done priorly in all cases makes sure this is not negative
> > so i am no sure what you meant
> > did i miss something ?
> > 
> > and yes it of course wont work with >8bit per sample or where a sample
> > crosses a byte boundary but i dont think there are any pixel formats that
> > would use the bitstream case and require these that we want to support.
> > If you know of a specific example pixel format that would fail and that
> > actually exists iam interrested ...
> 
> OK, thanks, it was exactly this which was puzzling me, maybe we should
> add a notice to the docs saying something like:
> 
> "read_line() will not work with bitstream formats with a depth per
> sample greater than 8, or where a sample crosses a byte boundary."
> 
> I'm attaching the updated hflip filter patch, which I'm using to test
> it (togheter with the pixdesc definition already posted).
> 
> Regards.
> -- 
> FFmpeg = Foolish Fostering Multimedia Portable Ecumenical Game

> Index: libavfilter-soc/ffmpeg/libavcodec/pixdesc.h
> ===================================================================
> --- libavfilter-soc.orig/ffmpeg/libavcodec/pixdesc.h	2009-04-19 13:17:27.000000000 +0200
> +++ libavfilter-soc/ffmpeg/libavcodec/pixdesc.h	2009-04-19 16:51:31.000000000 +0200
> @@ -137,3 +137,53 @@
>          }
>      }
>  }
> +
> +/**
> + * Writes the values from \p src to the pixel format components \p c
> + * of an image line.

\p ...




> + *
> + * @param data the array containing the pointers to the planes of the image
> + * @param linesizes the array containing the linesizes of the image
> + * @param desc the pixel format descriptor for the image
> + * @param x the horizontal coordinate of the first pixel to write
> + * @param y the vertical coordinate of the first pixel to write
> + * @param w the width of the line to write, that is the number of
> + * values to write to the image line
> + */
> +static inline void write_line(const uint16_t *src, uint8_t *data[4], const int linesize[4],
> +                              const AVPixFmtDescriptor *desc, int x, int y, int c, int w)

you are missing a param in the doxy


> +{
> +    AVComponentDescriptor comp = desc->comp[c];
> +    int plane = comp.plane;
> +    int depth = comp.depth_minus1+1;
> +    int step  = comp.step_minus1+1;
> +    int flags = desc->flags;
> +    uint32_t mask  = (1<<depth)-1;
> +
> +    if (flags & PIX_FMT_BITSTREAM) {

> +        int skip = x*step + comp.offset_plus1-1;
> +        uint8_t *p = data[plane] + y*linesize[plane] + (skip>>3);
> +        int shift = 8 - depth - (skip&7);
> +
> +        while (w--) {
> +            *p |= *src++ << shift;
> +            shift -= step;
> +            p -= shift>>3;
> +            shift &= 7;
> +        }

isnt this alot nicer than the redesigned bitstream writer? 



> +    } else {
> +        int shift = comp.shift;
> +        uint8_t *p = data[plane]+ y*linesize[plane] + x*step + comp.offset_plus1-1;
> +
> +        while (w--) {
> +            if (flags & PIX_FMT_BE) {
> +                uint16_t val = (AV_RB16(p) & ~(mask<<shift)) | (*src++<<shift);
> +                AV_WB16(p, val);
> +            } else {
> +                uint16_t val = (AV_RL16(p) & ~(mask<<shift)) | (*src++<<shift);

mask is useless, your initial buffer is supposed to be zeroed


> +                AV_WL16(p, val);
> +            }
> +            p+= step;
> +        }
> +    }
> +}

> Index: libavfilter-soc/ffmpeg/libavfilter/vf_hflip.c
> ===================================================================
> --- libavfilter-soc.orig/ffmpeg/libavfilter/vf_hflip.c	2009-04-19 17:04:08.000000000 +0200
> +++ libavfilter-soc/ffmpeg/libavfilter/vf_hflip.c	2009-04-19 17:13:36.000000000 +0200
[...]
> +static inline void write_line_flipped(const uint16_t *src, uint8_t *data[4], const int linesize[4],
> +                                      const AVPixFmtDescriptor *desc, int x, int y, int c, int w)
> +{
> +    AVComponentDescriptor comp = desc->comp[c];
> +    int plane = comp.plane;
> +    int depth = comp.depth_minus1+1;
> +    int step  = comp.step_minus1+1;
> +    int flags = desc->flags;
> +    uint32_t mask  = (1<<depth)-1;
> +
> +    src += w;
> +
> +    if (flags & PIX_FMT_BITSTREAM) {
> +        int skip = x*step + comp.offset_plus1-1;
> +        uint8_t *p = data[plane] + y*linesize[plane] + (skip>>3);
> +        int shift = 8 - depth - (skip&7);
> +
> +        while (w--) {
> +            *p |= *src-- << shift;
> +            shift -= step;
> +            p -= shift>>3;
> +            shift &= 7;
> +        }
> +    } else {
> +        int shift = comp.shift;
> +        uint8_t *p = data[plane]+ y*linesize[plane] + x*step + comp.offset_plus1-1;
> +
> +        while (w--) {
> +            if (flags & PIX_FMT_BE) {
> +                uint16_t val = (AV_RB16(p) & ~(mask<<shift)) | (*src--<<shift);
> +                AV_WB16(p, val);
> +            } else {
> +                uint16_t val = (AV_RL16(p) & ~(mask<<shift)) | (*src--<<shift);
> +                AV_WL16(p, val);
> +            }
> +            p+= step;
> +        }
> +    }
> +}
> +

this is just for testing?
either way, there is no chance i would approve this for svn.
for fliping of common formats its too slow and for uncommon ones
its far too ugly, you can fliped the "unpacked" line

[...]
-- 
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/20090419/d7e90d74/attachment.pgp>



More information about the ffmpeg-devel mailing list