| Message ID | CAB0OVGrbWKNrJX0ab2-AR3CDiK2yPGhqK37itEw7vujxSWu2bQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 8/26/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Hi! > > Attached patch slightly improves the saturation of the gdv palette. > > Please comment, Carl Eugen > Does this match how it is displayed by original game?
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, 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). Carl Eugen
2017-08-26 13:00 GMT+02:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>: > Hi! > > Attached patch slightly improves the saturation of the gdv palette. I'll push this if there are no objections. Carl Eugen
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? Ronald
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
From bbc00affb218ebe2b8074fea514bf25bbca5f890 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sat, 26 Aug 2017 12:59:11 +0200 Subject: [PATCH] lavf/gdv: Improve saturation of gdv palette. --- libavcodec/gdv.c | 3 +++ libavformat/gdv.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/libavcodec/gdv.c b/libavcodec/gdv.c index dc91869..009aace 100644 --- a/libavcodec/gdv.c +++ b/libavcodec/gdv.c @@ -433,6 +433,9 @@ static int gdv_decode_frame(AVCodecContext *avctx, void *data, 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; gdv->pal[i] = 0xFFU << 24 | r << 18 | g << 10 | b << 2; } break; diff --git a/libavformat/gdv.c b/libavformat/gdv.c index 3220932..98b073d 100644 --- a/libavformat/gdv.c +++ b/libavformat/gdv.c @@ -140,6 +140,9 @@ static int gdv_read_header(AVFormatContext *ctx) unsigned r = avio_r8(pb); unsigned g = avio_r8(pb); unsigned b = avio_r8(pb); + r |= r >> 4; + g |= g >> 4; + b |= b >> 4; gdv->pal[i] = 0xFFU << 24 | r << 18 | g << 10 | b << 2; } } -- 1.7.10.4