diff mbox series

[FFmpeg-devel] avfilter/vf_subtitles: allow using embedded fonts

Message ID 20210502210201.25045-1-oneric@oneric.de
State Accepted
Commit 3300625c6f148455b08d641597d54b5be4c0f76a
Headers show
Series [FFmpeg-devel] avfilter/vf_subtitles: allow using embedded fonts | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Oneric May 2, 2021, 9:02 p.m. UTC 
ASS subtitles can have encoded fonts embedded into the subtitle file
itself. Allow libass to load those, to render subs as intended.
---


A sample file for ASS with embedded font can be at the following link.
If everything works, the 'A' will render as a quad; see image:
https://raw.githubusercontent.com/TheOneric/libass-regression-tests-tmp/master/regression/embedded-font/efont.ass
https://raw.githubusercontent.com/TheOneric/libass-regression-tests-tmp/master/regression/embedded-font/efont-1000.png

Prior to libass commit 1140b6b885c89d37eef13dc1f31f144e9a76a4d7, included
in libass release 0.15.1, a libass-bug would have prevented this from actually working.
To test this a recent libass is therefore required.
'ass_extract_fonts' itself however has been part of libass' API ever since its
first standalone release, so calling this does not break compatibility with
older versions and at worst just nothing happens.
Use eg the follwoing to check:
  ./ffmpeg -f lavfi -i "color=cyan:640x480:d=5" -vf "ass=efont.ass" -f matroska - | ./ffplay -autoexit -

---
 libavfilter/vf_subtitles.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Oneric May 13, 2021, 3:45 p.m. UTC | #1 
On Sun, May 02, 2021 at 23:02:02 +0200, Oneric wrote:
> ASS subtitles can have encoded fonts embedded into the subtitle file
> itself. Allow libass to load those, to render subs as intended.
> ---
>  libavfilter/vf_subtitles.c | 1 +
>  1 file changed, 1 insertion(+)

pinging for review
Oneric May 21, 2021, 3:45 p.m. UTC | #2 
On Sun, May 02, 2021 at 23:02:02 +0200, Oneric wrote:
> ASS subtitles can have encoded fonts embedded into the subtitle file
> itself. Allow libass to load those, to render subs as intended.
> ---
>  libavfilter/vf_subtitles.c | 1 +
>  1 file changed, 1 insertion(+)

pinging again
Oneric May 28, 2021, 5:56 p.m. UTC | #3 
On Sun, May 02, 2021 at 23:02:02 +0200, Oneric wrote:
> ASS subtitles can have encoded fonts embedded into the subtitle file
> itself. Allow libass to load those, to render subs as intended.
> ---
>  libavfilter/vf_subtitles.c | 1 +
>  1 file changed, 1 insertion(+)

another ping
Gyan Doshi May 29, 2021, 5:37 a.m. UTC | #4 
On 2021-05-28 23:26, Oneric wrote:
> On Sun, May 02, 2021 at 23:02:02 +0200, Oneric wrote:
>> ASS subtitles can have encoded fonts embedded into the subtitle file
>> itself. Allow libass to load those, to render subs as intended.
>> ---
>>   libavfilter/vf_subtitles.c | 1 +
>>   1 file changed, 1 insertion(+)
> another ping

I'll test this and apply.

Gyan
Gyan Doshi May 29, 2021, 6:21 a.m. UTC | #5 
On 2021-05-29 11:07, Gyan Doshi wrote:
>
>
> On 2021-05-28 23:26, Oneric wrote:
>> On Sun, May 02, 2021 at 23:02:02 +0200, Oneric wrote:
>>> ASS subtitles can have encoded fonts embedded into the subtitle file
>>> itself. Allow libass to load those, to render subs as intended.
>>> ---
>>>   libavfilter/vf_subtitles.c | 1 +
>>>   1 file changed, 1 insertion(+)
>> another ping
>
> I'll test this and apply.

Pushed as 3300625c6f148455b08d641597d54b5be4c0f76a

Gyan
Gyan Doshi May 29, 2021, 6:23 a.m. UTC | #6 
On 2021-05-29 11:51, Gyan Doshi wrote:
>
>
> On 2021-05-29 11:07, Gyan Doshi wrote:
>>
>>
>> On 2021-05-28 23:26, Oneric wrote:
>>> On Sun, May 02, 2021 at 23:02:02 +0200, Oneric wrote:
>>>> ASS subtitles can have encoded fonts embedded into the subtitle file
>>>> itself. Allow libass to load those, to render subs as intended.
>>>> ---
>>>>   libavfilter/vf_subtitles.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>> another ping
>>
>> I'll test this and apply.
>
> Pushed as 3300625c6f148455b08d641597d54b5be4c0f76a

Would it make sense to allow users to not load embedded fonts?

Gyan
Oneric May 29, 2021, 4:44 p.m. UTC | #7 
On Sat, May 29, 2021 at 11:53:24 +0530, Gyan Doshi wrote:
> On 2021-05-29 11:51, Gyan Doshi wrote:
> > On 2021-05-29 11:07, Gyan Doshi wrote:
> > > I'll test this and apply.
> > 
> > Pushed as 3300625c6f148455b08d641597d54b5be4c0f76a
> 
> Would it make sense to allow users to not load embedded fonts?

Thanks!
I can't think of a reason not to load embedded fonts in ffmpeg; they are 
required to render subs as intended by the sub author. Without them some 
fallback font from the system or mkv-attachments will be used instead
(which if the requested font doesn't by chance happen to also be available
 on the system or as a container-attachment, will be some, more or
 less random font).
VLC always enables this unconditionally, but mplayer and mpv have an option 
for this defaulting to embedded fonts enabled, but as before I don't know why
anyone would want to disable this.


Oneric
Gyan Doshi May 29, 2021, 5:24 p.m. UTC | #8 
On 2021-05-29 22:14, Oneric wrote:
> On Sat, May 29, 2021 at 11:53:24 +0530, Gyan Doshi wrote:
>> On 2021-05-29 11:51, Gyan Doshi wrote:
>>> On 2021-05-29 11:07, Gyan Doshi wrote:
>>>> I'll test this and apply.
>>> Pushed as 3300625c6f148455b08d641597d54b5be4c0f76a
>> Would it make sense to allow users to not load embedded fonts?
> Thanks!
> I can't think of a reason not to load embedded fonts in ffmpeg; they are
> required to render subs as intended by the sub author. Without them some
> fallback font from the system or mkv-attachments will be used instead
> (which if the requested font doesn't by chance happen to also be available
>   on the system or as a container-attachment, will be some, more or
>   less random font).
> VLC always enables this unconditionally, but mplayer and mpv have an option
> for this defaulting to embedded fonts enabled, but as before I don't know why
> anyone would want to disable this.

Security reasons?

Regards,
Gyan
Oneric May 30, 2021, 7:20 p.m. UTC | #9 
On Sat, May 29, 2021 at 22:54:44 +0530, Gyan Doshi wrote:
> On 2021-05-29 22:14, Oneric wrote:
> > On Sat, May 29, 2021 at 11:53:24 +0530, Gyan Doshi wrote:
> > > Would it make sense to allow users to not load embedded fonts?
> >
> > I can't think of a reason not to load embedded fonts in ffmpeg; they are
> > required to render subs as intended by the sub author. Without them some
> > fallback font from the system or mkv-attachments will be used instead
> > (which if the requested font doesn't by chance happen to also be available
> >   on the system or as a container-attachment, will be some, more or
> >   less random font).
> > VLC always enables this unconditionally, but mplayer and mpv have an option
> > for this defaulting to embedded fonts enabled, but as before I don't know why
> > anyone would want to disable this.
> 
> Security reasons?

If one expects processing a font file to expose (relevantly) more
attack surface, or be more dangerous than the rest of untrusted ASS
files, than perhaps yes.
Otherwise, I at least can't think of something right now.

Cheers
Oneric
diff mbox series

Patch

diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
index 493eb5f424..ab32e1b7f3 100644
--- a/libavfilter/vf_subtitles.c
+++ b/libavfilter/vf_subtitles.c
@@ -111,6 +111,7 @@  static av_cold int init(AVFilterContext *ctx)
     ass_set_message_cb(ass->library, ass_log, ctx);
 
     ass_set_fonts_dir(ass->library, ass->fontsdir);
+    ass_set_extract_fonts(ass->library, 1);
 
     ass->renderer = ass_renderer_init(ass->library);
     if (!ass->renderer) {