[FFmpeg-devel] [PATCH 1/2] movtextenc: 3GPP TS 26.245 Timed Text Encoder.

[Philip Langdale]

On Thu, 26 Jul 2012 22:05:37 +0200
Clément Bœsch <ubitux at gmail.com> wrote:

> > +#include "movtext.h"
> 
> The patch is missing that file so I can't really test the patch.

This leaked in from my initial styling attempts. I'll remove it - 
although I just realised that I failed to remove it from my v2
diff. whoops.
 
> > +        'S', 'e', 'r', 'i', 'f',// uint8_t font[font-name-length]
> 
> nit: doesn't Serif look weird for subtitles?

Yes, but I followed the default style from MP4Box, and no player
respects it anyway :-)

> > +    avctx->extradata_size = sizeof text_sample_entry;
> 
> I don't think it has any impact in this particular case, but no need
> for an extra padding size?

I don't think so - what do you specifically mean? I'm not aware of any
power of two requirement here.
 
> note: I don't mind the missing () after sizeof but we tend to prefer
> with them.

I was always taught that you use () with types (because you have to)
and not with values, but I can change it.

> > +    avctx->extradata = av_mallocz(avctx->extradata_size);
> > +    if (!avctx->extradata)
> > +    	return AVERROR(ENOMEM);
> 
> You have a tab here

Fixed.
 
> 
> I maybe be missing something but are you really using this saved
> pointer somewhere?

No. It's unused. removed.

> > +static int mov_text_encode_frame(AVCodecContext *avctx,
> > +                            unsigned char *buf, int bufsize, void
> > *data)
> 
> nit: align

Fixed.
 
> > +    for (i=0; i<sub->num_rects; i++) {
> 
> nit: some spacing around operators

Fixed.
 
> 
> AVERROR(EINVAL) or something maybe.

Done.

Thanks!

--phil