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

[Alexandre Colucci]

On 30 avr. 2011, at 11:27, Reimar Döffinger wrote:

> 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.


That is not a maybe but a definite yes. There are also vobsub in mp4 and mov.


> Part of the issue
> is that FFmpeg does not support reading from DVD.
> 

That is not an issue at all. Libavcodec is a library, it can be used by tools that know how to read a 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.


You mean my first patch :-) I myself suggested to discard the second one.


> 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);


It's good but since you mention "simpler" the table is not needed at all, you can remove it entirely and replace the line:

level = (0xff * j) / nb_opaque_colors;

with:

level = (nb_opaque_colors > 1) ? (0xff * j) / (nb_opaque_colors - 1) : 0xff;


If you do the above change the patch will become identical to the first patch I posted in this thread
If you do not do the above change the patch will become *equivalent* to the first patch I posted in this thread


Alexandre