Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avcodec/dvdsubdec: fix incorrect yellow appearance of dvd subtitles #16

Closed
wants to merge 1 commit into from

Conversation

softworkz
Copy link
Collaborator

@softworkz softworkz commented Jan 4, 2022

Fixes an age-old bug in decoding DVD subtitles.

Ever wondered why certain DVD subtitles are shown in yellow color when ffmpeg
is involved...

cc: Soft Works softworkz@hotmail.com
cc: Scott Theisen scott.the.elm@gmail.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>
@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Jan 4, 2022

Submitted as pull.16.ffstaging.FFmpeg.1641262759164.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-16/softworkz/patch_dvdsubdec_fix-v1

To fetch this version to local tag pr-ffstaging-16/softworkz/patch_dvdsubdec_fix-v1:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-16/softworkz/patch_dvdsubdec_fix-v1

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 3, 2022

On the FFmpeg mailing list, Soft Works wrote (reply to this):



> -----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)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 3, 2022

User Soft Works <softworkz@hotmail.com> has been added to the cc: list.

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 3, 2022

On the FFmpeg mailing list, Scott Theisen wrote (reply to this):

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


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 3, 2022

User Scott Theisen <scott.the.elm@gmail.com> has been added to the cc: list.

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 3, 2022

On the FFmpeg mailing list, Soft Works wrote (reply to this):



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






_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 3, 2022

On the FFmpeg mailing list, Scott Theisen wrote (reply to this):

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented Feb 4, 2022

On the FFmpeg mailing list, Soft Works wrote (reply to this):



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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@softworkz
Copy link
Collaborator Author

Merged upstream

@softworkz softworkz closed this May 25, 2022
@softworkz softworkz deleted the patch_dvdsubdec_fix branch October 11, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant