[FFmpeg-devel] [PATCH] movtextdec.c: Add support for font names

[Philip Langdale]

On Thu, 6 Aug 2015 07:13:07 +0530
Niklesh Lalwani <lalwani1994 at gmail.com> wrote:

> On Thu, Aug 6, 2015 at 2:02 AM, Philip Langdale <philipl at overt.org>
> wrote:
> 
> > On Thu, 6 Aug 2015 00:12:07 +0530
> > Niklesh Lalwani <lalwani1994 at gmail.com> wrote:
> >
> > > From 90f466bb6a5d3cd24d7ea4d9fd8a2915cc68cdb2 Mon Sep 17 00:00:00
> > > 2001 From: Niklesh <niklesh.lalwani at iitb.ac.in>
> > > Date: Thu, 6 Aug 2015 00:06:15 +0530
> > > Subject: [PATCH] movtextdec.c: Add support for font names
> > >
> > > Signed-off-by: Niklesh <niklesh.lalwani at iitb.ac.in>
> > > ---
> > >  libavcodec/movtextdec.c | 109
> > > ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed,
> > > 106 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
> > > index 869358c..451056c 100644
> > > --- a/libavcodec/movtextdec.c
> > > +++ b/libavcodec/movtextdec.c
> > > @@ -36,10 +36,17 @@
> > >  #define HCLR_BOX   (1<<2)
> > >
> > >  typedef struct {
> > > +    uint16_t fontID;
> > > +    uint8_t font_name_length;
> > > +    uint8_t font[20];
> > > +} FontRecord;
> > > +
> > > +typedef struct {
> > >      uint16_t style_start;
> > >      uint16_t style_end;
> > >      uint8_t style_flag;
> > >      uint8_t fontsize;
> > > +    uint16_t style_fontID;
> > >  } StyleBox;
> > >
> > >  typedef struct {
> > > @@ -56,11 +63,13 @@ typedef struct {
> > >      StyleBox *s_temp;
> > >      HighlightBox h;
> > >      HilightcolorBox c;
> > > +    FontRecord **ftab;
> > > +    FontRecord *ftab_temp;
> > >      uint8_t box_flags;
> > > -    uint16_t style_entries;
> > > +    uint16_t style_entries, ftab_entries;
> > >      uint64_t tracksize;
> > >      int size_var;
> > > -    int count_s;
> > > +    int count_s, count_f;
> > >  } MovTextContext;
> > >
> > >  typedef struct {
> > > @@ -80,6 +89,80 @@ static void mov_text_cleanup(MovTextContext *m)
> > >      }
> > >  }
> > >
> > > +static void mov_text_cleanup_ftab(MovTextContext *m)
> > > +{
> > > +    int i;
> > > +    for(i = 0; i < m->count_f; i++) {
> > > +        av_freep(&m->ftab[i]);
> > > +    }
> > > +    av_freep(&m->ftab);
> > > +}
> > > +
> > > +static int mov_text_t3xg(AVCodecContext *avctx, MovTextContext
> > > *m)
> >
> > I'm pretty sure it's "tx3g"
> >
> > > +{
> > > +    char *t3xg_ptr = avctx->extradata;
> > > +    int i, tracksize;
> > > +
> > > +    tracksize = 38; /* Size till ftab_entries */
> >
> > I'd say box size. It's not technically a track.
> >
> > And make the 38 a constant.
> >
> > > +    if (avctx->extradata_size < tracksize)
> > > +        return -1;
> > > +
> > > +    m->ftab_entries = 0;
> > > +    // Display Flags
> > > +    t3xg_ptr += 4;
> > > +    // Alignment
> > > +    t3xg_ptr += 2;
> > > +    // Background Color
> > > +    t3xg_ptr += 4;
> > > +    // BoxRecord
> > > +    t3xg_ptr += 8;
> > > +    // StyleRecord
> > > +    t3xg_ptr += 12;
> > > +    // FontRecord
> > > +    // FontRecord Size
> > > +    t3xg_ptr += 4;
> > > +    // ftab
> > > +    t3xg_ptr += 4;
> > > +
> > > +    tracksize += 2;
> > > +    if (avctx->extradata_size < tracksize)
> > > +        return -1;
> >
> > Why not just initialize tracksize to 40? The extra += 2 isn't
> > optional.
> >
> > > +
> > > +    m->ftab_entries = AV_RB16(t3xg_ptr);
> > > +    t3xg_ptr += 2;
> > > +
> > > +    for (i = 0; i < m->ftab_entries; i++) {
> > > +
> > > +        tracksize += 3;
> > > +        if (avctx->extradata_size < tracksize) {
> > > +            m->ftab_entries = 0;
> > > +            return -1;
> > > +        }
> >
> > You need to clean up any ftab entries that have already been
> > processed.
> >
> > > +        m->ftab_temp = av_malloc(sizeof(*m->ftab_temp));
> > > +        if (!m->ftab_temp) {
> > > +            mov_text_cleanup_ftab(m);
> > > +            return AVERROR(ENOMEM);
> > > +        }
> > > +        m->ftab_temp->fontID = AV_RB16(t3xg_ptr);
> > > +        t3xg_ptr += 2;
> > > +        m->ftab_temp->font_name_length = *t3xg_ptr++;
> > > +
> > > +        tracksize = tracksize + m->ftab_temp->font_name_length;
> > > +        if ((avctx->extradata_size < tracksize) ||
> > > (m->ftab_temp->font_name_length > 20)) {
> > > +            m->ftab_entries = 0;
> > > +            return -1;
> > > +        }
> > > +        memcpy(m->ftab_temp->font, t3xg_ptr,
> > > m->ftab_temp->font_name_length);
> > > +        av_dynarray_add(&m->ftab, &m->count_f, m->ftab_temp);
> > > +        if (!m->ftab) {
> > > +            mov_text_cleanup_ftab(m);
> > > +            return AVERROR(ENOMEM);
> > > +        }
> >
> > I didn't think about this before, but there's no need to store the
> > font name and length in your FontRecord in this way. Just store it
> > as a normal null terminated string. Then you don't need the size
> > and do can do normal string manipulation on it later (like using
> > av_bprintf with '%s').
> >
> 
> Yes, I can do that and add a null character at the end after memcopy,
> but I thought I'll define the structs as close as possible to the
> Records defined in 3GPP spec.
> Also, I think I should not limit the length to 20. Ideally, it is a
> uint8_t value. In that case, I'll need to malloc the size of font
> length.

Oh my. I thought the 20 character limit was in the spec. If the spec
has no limit, then you can't have one either - you do need to malloc
and append a null. Thanks.

> 
> Rest all updated.
> 
> Thanks,
> Niklesh
> 
> 
> > > +        t3xg_ptr = t3xg_ptr + 3 + m->ftab[i]->font_name_length;
> > > +    }
> >
> >




--phil