diff mbox series

[FFmpeg-devel] avcodec/dvdsubdec: fix incorrect yellow appearance of dvd subtitles

Message ID pull.16.ffstaging.FFmpeg.1641262759164.ffmpegagent@gmail.com
State Accepted
Commit 58b07ecb3fdbb0f5c55cd482610d12bda69fed20
Headers show
Series [FFmpeg-devel] avcodec/dvdsubdec: fix incorrect yellow appearance of dvd subtitles | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc fail Make fate failed

Commit Message

softworkz Jan. 4, 2022, 2:19 a.m. UTC
From: softworkz <softworkz@hotmail.com>

The guess_palette() implementation is questionable in itself
as its results don't match those from other DVD subtitle decoders.

This commit starts cleanup by fixing an obvious bug which has made
certain DVD subs appear yellow instead of white or grey for more than
10 years..

Signed-off-by: softworkz <softworkz@hotmail.com>
---
    avcodec/dvdsubdec: fix incorrect yellow appearance of dvd subtitles
    
    Fixes an age-old bug in decoding DVD subtitles.
    
    Ever wondered why certain DVD subtitles are shown in yellow color when
    ffmpeg is involved...

Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-16%2Fsoftworkz%2Fpatch_dvdsubdec_fix-v1
Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-16/softworkz/patch_dvdsubdec_fix-v1
Pull-Request: https://github.com/ffstaging/FFmpeg/pull/16

 libavcodec/dvdsubdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 573b6b8a607398c5f34108efda9c29d41c5727ff

Comments

Soft Works Feb. 3, 2022, 10:10 p.m. UTC | #1
> -----Original Message-----
> From: ffmpegagent <ffmpegagent@gmail.com>
> Sent: Tuesday, January 4, 2022 3:19 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: softworkz <softworkz@hotmail.com>; softworkz
> <softworkz@hotmail.com>
> Subject: [PATCH] avcodec/dvdsubdec: fix incorrect yellow appearance of
> dvd subtitles
> 
> From: softworkz <softworkz@hotmail.com>
> 
> The guess_palette() implementation is questionable in itself
> as its results don't match those from other DVD subtitle decoders.
> 
> This commit starts cleanup by fixing an obvious bug which has made
> certain DVD subs appear yellow instead of white or grey for more than
> 10 years..
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>     avcodec/dvdsubdec: fix incorrect yellow appearance of dvd
> subtitles
> 
>     Fixes an age-old bug in decoding DVD subtitles.
> 
>     Ever wondered why certain DVD subtitles are shown in yellow color
> when
>     ffmpeg is involved...
> 
> Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-
> ffstaging-16%2Fsoftworkz%2Fpatch_dvdsubdec_fix-v1
> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
> ffstaging-16/softworkz/patch_dvdsubdec_fix-v1
> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/16
> 
>  libavcodec/dvdsubdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/dvdsubdec.c b/libavcodec/dvdsubdec.c
> index 52259f0730..a3fdb535a5 100644
> --- a/libavcodec/dvdsubdec.c
> +++ b/libavcodec/dvdsubdec.c
> @@ -400,7 +400,7 @@ static int decode_dvd_subtitles(DVDSubContext
> *ctx, AVSubtitle *sub_header,
>                  } else {
>                      sub_header->rects[0]->nb_colors = 4;
>                      guess_palette(ctx, (uint32_t*)sub_header-
> >rects[0]->data[1],
> -                                  0xffff00);
> +                                  0xffffff);
>                  }
>                  sub_header->rects[0]->x = x1;
>                  sub_header->rects[0]->y = y1;
> 
> base-commit: 573b6b8a607398c5f34108efda9c29d41c5727ff
> --
> ffmpeg-codebot

Ping. (no maintainer seems to be registered for this)
Scott Theisen Feb. 3, 2022, 10:16 p.m. UTC | #2
On 2/3/22 17:10, Soft Works wrote:
>
>> -----Original Message-----
>> From: ffmpegagent <ffmpegagent@gmail.com>
>> Sent: Tuesday, January 4, 2022 3:19 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: softworkz <softworkz@hotmail.com>; softworkz
>> <softworkz@hotmail.com>
>> Subject: [PATCH] avcodec/dvdsubdec: fix incorrect yellow appearance of
>> dvd subtitles
>>
>> From: softworkz <softworkz@hotmail.com>
>>
>> The guess_palette() implementation is questionable in itself
>> as its results don't match those from other DVD subtitle decoders.
>>
>> This commit starts cleanup by fixing an obvious bug which has made
>> certain DVD subs appear yellow instead of white or grey for more than
>> 10 years..
>>
>> Signed-off-by: softworkz <softworkz@hotmail.com>
>> ---
>>      avcodec/dvdsubdec: fix incorrect yellow appearance of dvd
>> subtitles
>>
>>      Fixes an age-old bug in decoding DVD subtitles.
>>
>>      Ever wondered why certain DVD subtitles are shown in yellow color
>> when
>>      ffmpeg is involved...
>>
>> Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-
>> ffstaging-16%2Fsoftworkz%2Fpatch_dvdsubdec_fix-v1
>> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
>> ffstaging-16/softworkz/patch_dvdsubdec_fix-v1
>> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/16
>>
>>   libavcodec/dvdsubdec.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/dvdsubdec.c b/libavcodec/dvdsubdec.c
>> index 52259f0730..a3fdb535a5 100644
>> --- a/libavcodec/dvdsubdec.c
>> +++ b/libavcodec/dvdsubdec.c
>> @@ -400,7 +400,7 @@ static int decode_dvd_subtitles(DVDSubContext
>> *ctx, AVSubtitle *sub_header,
>>                   } else {
>>                       sub_header->rects[0]->nb_colors = 4;
>>                       guess_palette(ctx, (uint32_t*)sub_header-
>>> rects[0]->data[1],
>> -                                  0xffff00);
>> +                                  0xffffff);
>>                   }
>>                   sub_header->rects[0]->x = x1;
>>                   sub_header->rects[0]->y = y1;
>>
>> base-commit: 573b6b8a607398c5f34108efda9c29d41c5727ff
>> --
>> ffmpeg-codebot
> Ping. (no maintainer seems to be registered for this)

MythTV has used this fix since 2010-06-04.  See 
https://github.com/ulmus-scott/FFmpeg/commit/e2b1a6ee63c0cccc1ac9c82d24e2e6cfffeb2bfc

libavcodec/dvdsubdec.c: default to white instead of yellow

from Improved display of DVD subtitles in containers other than the 
original. 
https://github.com/MythTV/mythtv/commit/2510a0821ea4453eb9b34dd96e68ff0441459d0b
references: https://code.mythtv.org/trac/ticket/8222
For whatever strange reason, ffmpeg has always used yellow: 
https://github.com/FFmpeg/FFmpeg/commit/240c1657dcd45adc0e63ef947b920919071ec1f7
Soft Works Feb. 3, 2022, 10:57 p.m. UTC | #3
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Scott Theisen
> Sent: Thursday, February 3, 2022 11:17 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/dvdsubdec: fix incorrect
> yellow appearance of dvd subtitles
> 
> On 2/3/22 17:10, Soft Works wrote:
> >
> >> -----Original Message-----
> >> From: ffmpegagent <ffmpegagent@gmail.com>
> >> Sent: Tuesday, January 4, 2022 3:19 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Cc: softworkz <softworkz@hotmail.com>; softworkz
> >> <softworkz@hotmail.com>
> >> Subject: [PATCH] avcodec/dvdsubdec: fix incorrect yellow appearance
> of
> >> dvd subtitles
> >>
> >> From: softworkz <softworkz@hotmail.com>
> >>
> >> The guess_palette() implementation is questionable in itself
> >> as its results don't match those from other DVD subtitle decoders.
> >>
> >> This commit starts cleanup by fixing an obvious bug which has made
> >> certain DVD subs appear yellow instead of white or grey for more
> than
> >> 10 years..
> >>
> >> Signed-off-by: softworkz <softworkz@hotmail.com>
> >> ---
> >>      avcodec/dvdsubdec: fix incorrect yellow appearance of dvd
> >> subtitles
> >>
> >>      Fixes an age-old bug in decoding DVD subtitles.
> >>
> >>      Ever wondered why certain DVD subtitles are shown in yellow
> color
> >> when
> >>      ffmpeg is involved...
> >>
> >> Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-
> >> ffstaging-16%2Fsoftworkz%2Fpatch_dvdsubdec_fix-v1
> >> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
> >> ffstaging-16/softworkz/patch_dvdsubdec_fix-v1
> >> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/16
> >>
> >>   libavcodec/dvdsubdec.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/dvdsubdec.c b/libavcodec/dvdsubdec.c
> >> index 52259f0730..a3fdb535a5 100644
> >> --- a/libavcodec/dvdsubdec.c
> >> +++ b/libavcodec/dvdsubdec.c
> >> @@ -400,7 +400,7 @@ static int decode_dvd_subtitles(DVDSubContext
> >> *ctx, AVSubtitle *sub_header,
> >>                   } else {
> >>                       sub_header->rects[0]->nb_colors = 4;
> >>                       guess_palette(ctx, (uint32_t*)sub_header-
> >>> rects[0]->data[1],
> >> -                                  0xffff00);
> >> +                                  0xffffff);
> >>                   }
> >>                   sub_header->rects[0]->x = x1;
> >>                   sub_header->rects[0]->y = y1;
> >>
> >> base-commit: 573b6b8a607398c5f34108efda9c29d41c5727ff
> >> --
> >> ffmpeg-codebot
> > Ping. (no maintainer seems to be registered for this)
> 
> MythTV has used this fix since 2010-06-04.  See
> https://github.com/ulmus-
> scott/FFmpeg/commit/e2b1a6ee63c0cccc1ac9c82d24e2e6cfffeb2bfc
> 
> libavcodec/dvdsubdec.c: default to white instead of yellow
> 
> from Improved display of DVD subtitles in containers other than the
> original.
> https://github.com/MythTV/mythtv/commit/2510a0821ea4453eb9b34dd96e68ff
> 0441459d0b
> references: https://code.mythtv.org/trac/ticket/8222

Thanks for the references. I have compared the ffmpeg output
with a range of (non-ffmpeg-based) players including VLC, 
an example where I had a screen photo from a hardware player
and two subtitle editing tools. 
None of those showed them yellow.


> For whatever strange reason, ffmpeg has always used yellow:
> https://github.com/FFmpeg/FFmpeg/commit/240c1657dcd45adc0e63ef947b9209
> 19071ec1f7

My suspicion is that 0xffff00 was written assuming RGBA order instead
of ARGB.

I'm pretty sure that it's a mistake and it hasn't been an intentional
choice, because even when you would consider a yellow color to for
whatever reason, it wouldn't have been exactly this yellow (#ff0),
because its lightness/saturation values are not in a range of what
is suitable for colored subtitles.

softworkz
Scott Theisen Feb. 3, 2022, 11:14 p.m. UTC | #4
On 2/3/22 17:57, Soft Works wrote:
> My suspicion is that 0xffff00 was written assuming RGBA order instead
> of ARGB.

You are missing a byte for either RGBA or ARGB.  RGBA would be cyan.
> I'm pretty sure that it's a mistake and it hasn't been an intentional
> choice, because even when you would consider a yellow color to for
> whatever reason, it wouldn't have been exactly this yellow (#ff0),
> because its lightness/saturation values are not in a range of what
> is suitable for colored subtitles.

I think using yellow for debugging  purposes is more likely, i.e. it is 
obvious if the pallet was missing/not detected.  However, it shouldn't 
have been committed like that, if that was the reason.

Regards,
Scott
Soft Works Feb. 4, 2022, 12:05 a.m. UTC | #5
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Scott Theisen
> Sent: Friday, February 4, 2022 12:15 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/dvdsubdec: fix incorrect
> yellow appearance of dvd subtitles
> 
> On 2/3/22 17:57, Soft Works wrote:
> > My suspicion is that 0xffff00 was written assuming RGBA order
> instead
> > of ARGB.
> 
> You are missing a byte for either RGBA or ARGB.  RGBA would be cyan.
> I think using yellow for debugging  purposes is more likely, i.e. it

Something made me think that it might have come
from a byte ordering issue when I had debugged an example, but 
I don't remember exactly. Probably you are just right about it, but 
only Fabrice might be able to tell for sure..

> However, it shouldn't
> have been committed like that, if that was the reason.

No matter the reason, I don't think there's any doubt that it's wrong?

softworkz
diff mbox series

Patch

diff --git a/libavcodec/dvdsubdec.c b/libavcodec/dvdsubdec.c
index 52259f0730..a3fdb535a5 100644
--- a/libavcodec/dvdsubdec.c
+++ b/libavcodec/dvdsubdec.c
@@ -400,7 +400,7 @@  static int decode_dvd_subtitles(DVDSubContext *ctx, AVSubtitle *sub_header,
                 } else {
                     sub_header->rects[0]->nb_colors = 4;
                     guess_palette(ctx, (uint32_t*)sub_header->rects[0]->data[1],
-                                  0xffff00);
+                                  0xffffff);
                 }
                 sub_header->rects[0]->x = x1;
                 sub_header->rects[0]->y = y1;