diff mbox series

[FFmpeg-devel,v2] lavf/gdv: Improve palette saturation

Message ID CAB0OVGqF_GpjjGVht1z0ncVR-2x+g8MM5qeVHgfpqsoH9StOSQ@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] lavf/gdv: Improve palette saturation | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Carl Eugen Hoyos April 12, 2020, 8:46 p.m. UTC 
Am Do., 7. Sept. 2017 um 15:40 Uhr schrieb Ronald S. Bultje
<rsbultje@gmail.com>:
>
> Hi Carl,
>
> On Mon, Aug 28, 2017 at 10:49 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>
> > 2017-08-27 9:03 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
> > > On 8/26/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> > >> Attached patch slightly improves the saturation of the
> > >> gdv palette.
> > >>
> > >> Please comment, Carl Eugen
> > >
> > > Does this match how it is displayed by original game?
> >
> > The original game was written for a graphic adapter that
> > supports 6bit palette. FFmpeg only supports 8bit palette,
> >
>
> OK, so that matches the current code:
>
>              unsigned r = bytestream2_get_byte(gb);
>              unsigned g = bytestream2_get_byte(gb);
>              unsigned b = bytestream2_get_byte(gb);
>              gdv->pal[i] = 0xFFU << 24 | r << 18 | g << 10 | b << 2;
>
> This indeed suggests the byte only contains 6 lower bits, the upper 2 being
> 0. It also indeed suggests that "white" (111111) would be converted to (in
> bits) 11111100, not 11111111, which is indeed unsaturated.
>
>
> > the patch makes the colour white (and all saturated colours)
> > as similar to the intended output as possible and does not
> > change black (and other colours with little intensity).
>
>
> So let's say you have (in bits) 111111 or 000000 (white and black).
>
>              unsigned r = bytestream2_get_byte(gb);
>              unsigned g = bytestream2_get_byte(gb);
>              unsigned b = bytestream2_get_byte(gb);
> +            r |= r >> 4;
> +            g |= g >> 4;
> +            b |= b >> 4;
>
> This converts that to 111111 and 000000.
>
>              gdv->pal[i] = 0xFFU << 24 | r << 18 | g << 10 | b << 2;
>
> And it seems to me that the color is then _still_ 11111100, which is
> unsaturated. Don't you want to do something like:
>
> #define convert6to8(x) ((x << 2) | ((x + 8) >> 4))
>
>              unsigned r = bytestream2_get_byte(gb);
>              unsigned g = bytestream2_get_byte(gb);
>              unsigned b = bytestream2_get_byte(gb);
> +            r = convert6to8(r);
> +            g = convert6to8(g);
> +            b = convert6to8(b);
>              gdv->pal[i] = (0xFFU << 24) | (r << 16) | (g << 8) | b;
>
> Or am I misunderstanding?

No, thank you for your review!

New patch attached.

Please comment, Carl Eugen
diff mbox series

Patch

From 3e456735e8dd268947a92574296a80a8a1e40750 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Sun, 12 Apr 2020 22:46:08 +0200
Subject: [PATCH] lavf/gdv: Improve saturation of gdv palette.

---
 libavformat/gdv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/gdv.c b/libavformat/gdv.c
index 2ecbb535e7..2d50530b83 100644
--- a/libavformat/gdv.c
+++ b/libavformat/gdv.c
@@ -143,7 +143,7 @@  static int gdv_read_header(AVFormatContext *ctx)
             unsigned r = avio_r8(pb);
             unsigned g = avio_r8(pb);
             unsigned b = avio_r8(pb);
-            gdv->pal[i] = 0xFFU << 24 | r << 18 | g << 10 | b << 2;
+            gdv->pal[i] = 0xFFU << 24 | r << 18 | r >> 4 << 16 | g << 10 | g >> 4 << 8 | b << 2 | b >> 4;
         }
     }
 
-- 
2.24.1