[FFmpeg-devel] [PATCH] lavfi/subtitles: load attached fonts to libass.

[wm4]

On Tue, 8 Apr 2014 17:37:01 -0300
Facundo Gaich <facugaich at gmail.com> wrote:

> On Apr 8, 2014 10:07 AM, "wm4" <nfxjfg at googlemail.com> wrote:
> >
> > On Tue, 8 Apr 2014 14:42:52 +0200
> > Clément Boesch <u at pkh.me> wrote:
> >
> > > On Mon, Apr 07, 2014 at 11:35:01PM -0300, Facundo Gaich wrote:
> > > > Videos with complex typesetting usually have font files embedded
> > > > as attachment streams. vf_subtitles now finds all attachment
> > > > streams with a MIME type associated with fonts and loads them
> > > > to libass so it can use them for rendering.
> > > >
> > > > The code was basically ported from mpv's loadfile.c at 929793be7
> > > >
> > > > Signed-off-by: Facundo Gaich <facugaich at gmail.com>
> > > > ---
> > > >  libavfilter/vf_subtitles.c | 53
> +++++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 52 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
> > > > index e44f61d..8586304 100644
> > > > --- a/libavfilter/vf_subtitles.c
> > > > +++ b/libavfilter/vf_subtitles.c
> > > > @@ -249,11 +249,34 @@ static const AVOption subtitles_options[] = {
> > > >      {NULL},
> > > >  };
> > > >
> > > > +static const char *font_mimetypes[] = {
> > > > +    "application/x-truetype-font",
> > > > +    "application/vnd.ms-opentype",
> > > > +    "application/x-font-ttf",
> > > > +    "application/x-font",
> > > > +    NULL
> >
> > Looks exactly like the font mime type list in mpv.
> 
> Are you suggesting a copyright issue (mpv being GPLv2 or later)? I'm not
> familiar with how these are usually handled when the code is so trivial (or
> in general)

No, IMO too trivial. But you didn't copy the comment about the x-font
mime type.

> > Note that
> > 'application/x-font' probably never existed officially (e.g. it's not
> > in Haali's matroska demuxer), and is probably wrong. On the other hand,
> > it's probably not harmful to have this entry.
> >
> 
> Considering I went with a much less tolerant version of the mpv code (i.e.
> I don't check filename extension in the absence of mimetype) I guess I
> could leave out that case too.
> 
> > > > +};
> > > > +
> > > > +static int attachment_is_font(AVStream * st)
> > > > +{
> > > > +    const AVDictionaryEntry *tag = NULL;
> > > > +
> > > > +    tag = av_dict_get(st->metadata, "mimetype", NULL,
> AV_DICT_MATCH_CASE);
> > > > +
> > > > +    if (tag) {
> > > > +        for (int n = 0; font_mimetypes[n]; n++) {
> > >
> > > int should be out of the loop (it's not supported by all the compilers
> > > FFmpeg supports).
> > >
> > > > +            if (strcmp(font_mimetypes[n], tag->value) == 0)
> >
> > I think mime types are usually not case sensitive, so this catches
> > fonts with lowercase mime types only.
> >
> 
> Noted
> 
> > > > +                return 1;
> > > > +        }
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > +
> > > >  AVFILTER_DEFINE_CLASS(subtitles);
> > > >
> > > >  static av_cold int init_subtitles(AVFilterContext *ctx)
> > > >  {
> > > > -    int ret, sid;
> > > > +    int ret, sid, loaded_fonts;
> > > >      AVDictionary *codec_opts = NULL;
> > > >      AVFormatContext *fmt = NULL;
> > > >      AVCodecContext *dec_ctx = NULL;
> > > > @@ -293,6 +316,34 @@ static av_cold int
> init_subtitles(AVFilterContext *ctx)
> > > >      sid = ret;
> > > >      st = fmt->streams[sid];
> > > >
> > > > +    /* Load attached fonts */
> > > > +    for (int j = 0; j < fmt->nb_streams; j++) {
> > >
> > > ditto
> > >
> > > > +        AVStream *st = fmt->streams[j];
> > > > +        if ((st->codec->codec_type == AVMEDIA_TYPE_ATTACHMENT) &&
> > >
> > > Extra parenthesis
> > >
> > > > +            attachment_is_font(st)) {
> > > > +            const AVDictionaryEntry *tag = NULL;
> > > > +            tag = av_dict_get(st->metadata, "filename", NULL,
> > > > +                              AV_DICT_MATCH_CASE);
> > > > +
> > > > +            if (tag) {
> > > > +                av_log(ctx, AV_LOG_DEBUG, "Loading attached font:
> %s\n",
> > > > +                       tag->value);
> > > > +                ass_add_font(ass->library, tag->value,
> > > > +                             st->codec->extradata,
> > > > +                             st->codec->extradata_size);
> > > > +                loaded_fonts = 1;
> > > > +            } else {
> > > > +                av_log(ctx, AV_LOG_WARNING,
> > > > +                       "Font attachment has no filename,
> ignored.\n");
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +    if (loaded_fonts) {
> > > > +        /* Ideally this would be ass_fonts_update, but it seems to
> be broken
> > > > +         * as of libass 0.11.1 */
> > >
> > > Can you add a bug id or something for reference in the comment?
> >
> > IMO not really a bug. ass_fonts_update() literally just calls
> > FcConfigBuildFonts, i.e. it updates the system fonts.
> > ass_set_fonts on the other hand adds fonts from ASS_Library to
> > ASS_Renderer.
> >
> > (It's worth noticing that libass wants to remove the separation between
> > ASS_Library and ASS_Renderer, so the need for the API call will
> > probably be removed once that is done.)
> >
> 
> Maybe calling it broken was an overstatement, if indeed that's the intended
> function of ass_fonts_update, the fault lies on a lack of documentation/API
> clarity. Anyways, considering the API will change in the future, is it ok
> if I change the comment to indicate that's the correct API call to force
> reloading of in memory fonts?

I guess so... By the way, I noticed that there's another ass_set_fonts()
call in init(), and this code is shared between the "ass" and
"subtitles" filter. Normally, you'd have to call ass_set_fonts() only
exactly once to initialize all font things.

> > > > +        ass_set_fonts(ass->renderer, NULL, NULL, 1, NULL, 1);
> > > > +    }
> > > > +
> > > >      /* Open decoder */
> > > >      dec_ctx = st->codec;
> > > >      dec = avcodec_find_decoder(dec_ctx->codec_id);
> > >
> > > Otherwise it looks OK.
> > >
> >
> 
> Thanks for the corrections, v2 coming soon.
> 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel