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