[FFmpeg-devel] [PATCH] Fix colors for DVD subtitles

[Reimar Döffinger]

On Sat, Apr 30, 2011 at 11:04:56AM +0200, Alexandre Colucci wrote:
> - the meaning of color indexes is : 00 (background), 01 (pattern), 10 (emphasis-1), 11 (emphasis-2). It is more important to support actual commercial DVDs than some homegrown tool's output where the author did not ever read the specifications.

You were the only one mentioning "homegrown tool". That you continue to insist
using this as argument despite that I've shown a few commercial DVDs
not doing it that way is really annoying!

> - we need to accept the fact that guess_palette can be wrong. There should really be no guessing, only in an extreme situation. In a future patch dvdsubdec should really look at the extra_data in avctx just like vlc and others do.

There is nothing in extradata, except maybe for MKV files. Part of the issue
is that FFmpeg does not support reading from DVD.

> - we need to pass a color to guess_palette and use the full scale for maximum contrast. For that reason white is better than yellow because black-white is greater contrast than black-yellow

Fine by me if nobody else has objections. I do find yellow somewhat annoying,
but it has been quite popular for some reason, e.g. most of the first DVD
players to support .avi with external srt subtitles seem to have chosen yellow
for some reason.

> All of the above is being taken care of in the first patch submitted in this thread.

If you don't mind I'd like something simpler/more obvious/easier to change.
Would everyone be ok with below patch (I'll apply the colour change separate
if nobody objects).


diff --git a/libavcodec/dvdsubdec.c b/libavcodec/dvdsubdec.c
index bb3e124..9b3c242 100644
--- a/libavcodec/dvdsubdec.c
+++ b/libavcodec/dvdsubdec.c
@@ -120,6 +120,14 @@ static void guess_palette(uint32_t *rgba_palette,
                           uint8_t *alpha,
                           uint32_t subtitle_color)
 {
+    static const uint8_t level_map[4][4] = {
+        // this configuration (full range, lowest to highest) in tests
+        // seemed most common, so assume this
+        {0xff},
+        {0x00, 0xff},
+        {0x00, 0x80, 0xff},
+        {0x00, 0x55, 0xaa, 0xff},
+    };                                                                                                                                
     uint8_t color_used[16];
     int nb_opaque_colors, i, level, j, r, g, b;
 
@@ -138,18 +146,18 @@ static void guess_palette(uint32_t *rgba_palette,
     if (nb_opaque_colors == 0)
         return;
 
-    j = nb_opaque_colors;
+    j = 0;
     memset(color_used, 0, 16);
     for(i = 0; i < 4; i++) {
         if (alpha[i] != 0) {
             if (!color_used[colormap[i]])  {
-                level = (0xff * j) / nb_opaque_colors;
+                level = level_map[nb_opaque_colors][j];
                 r = (((subtitle_color >> 16) & 0xff) * level) >> 8;
                 g = (((subtitle_color >> 8) & 0xff) * level) >> 8;
                 b = (((subtitle_color >> 0) & 0xff) * level) >> 8;
                 rgba_palette[i] = b | (g << 8) | (r << 16) | ((alpha[i] * 17) << 24);
                 color_used[colormap[i]] = (i + 1);
-                j--;
+                j++;
             } else {
                 rgba_palette[i] = (rgba_palette[color_used[colormap[i]] - 1] & 0x00ffffff) |
                                     ((alpha[i] * 17) << 24);