[Ffmpeg-devel] [PATCH, RFC] 16-bit grayscale depth support suite

Michael Niedermayer michaelni
Sun Oct 22 16:06:22 CEST 2006


Hi

now the actual review of the change 

On Sat, Oct 21, 2006 at 03:39:47PM +0300, Kostya wrote:
> Here is support for grayscale 16-bit and some trivial patches
> for 16-bit support in some formats. Comments, suggestions and
> SWScale support is welcomed ;)

> Index: libavutil/avutil.h
> ===================================================================
> --- libavutil/avutil.h	(revision 6734)
> +++ libavutil/avutil.h	(working copy)
> @@ -105,6 +105,8 @@
>      PIX_FMT_RGB32_1,   ///< Packed RGB 8:8:8, 32bpp, (msb)8R 8G 8B 8A(lsb), in cpu endianness
>      PIX_FMT_BGR32_1,   ///< Packed RGB 8:8:8, 32bpp, (msb)8B 8G 8R 8A(lsb), in cpu endianness
>  
> +    PIX_FMT_GRAY16BE,  ///<        Y         , 16bpp, big-endian
> +    PIX_FMT_GRAY16LE,  ///<        Y         , 16bpp, little-endian

the ',' isnt aligned


[...]
> +static void gray_to_gray16le(AVPicture *dst, const AVPicture *src,
> +                              int width, int height)
> +{
> +    int x, y, src_wrap, dst_wrap;
> +    uint8_t *s, *d;
> +    s = src->data[0];
> +    src_wrap = src->linesize[0] - width;
> +    d = dst->data[0];
> +    dst_wrap = dst->linesize[0] - width * 2;
> +    for(y=0; y<height; y++){
> +        for(x=0; x<width; x++){
> +            *d++ = 0;
> +            *d++ = *s++;

this can be wrong depending on how the digital value -> brightness is defined
if 0,0 = black; 255,65535=white then
    int t= *s++;
    *d++= t;
    *d++= t;
is correcter

the difference is that white (255 = 100%) becomes (65280 =99.6%) in your
version, this is also more noticeable with fewer bits 
(240 vs. 255 with 4->8bit and 128 vs 255 for 1->8bit)

as a sideeffect the output endianness wouldnt matter anymore

if 0,0 = black 256,65536=white then your version would be correct

if -0.5,-0.5 = black 255.5 65535.5=white then
    *d++ = 128;
    *d++ = *s++;
should be correct


[...]
> +static void gray16be_to_gray(AVPicture *dst, const AVPicture *src,
> +                              int width, int height)
> +{
> +    int x, y, src_wrap, dst_wrap;
> +    uint8_t *s, *d;
> +    s = src->data[0];
> +    src_wrap = src->linesize[0] - width * 2;
> +    d = dst->data[0];
> +    dst_wrap = dst->linesize[0] - width;
> +    for(y=0; y<height; y++){
> +        for(x=0; x<width; x++){
> +            *d++ = *s++;
> +            s++;

if 0,0 = black; 255,65535=white then
out = (in + 128)/257 (x/257 ~ (x-(x>>8))>>8)
would be correct

if 0,0 = black 256,65536=white then
out = FFMIN((in + 128)>>8, 255)
would be correct

if -0.5,-0.5 = black 255.5 65535.5=white then your version seems correct

IMHO the 8->16 and 16->8 should use the same definitions ...


[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is




More information about the ffmpeg-devel mailing list