[FFmpeg-devel] [PATCH] lavf/mxfdec: handle identification metadata

Tomas Härdin tomas.hardin at codemill.se
Mon Apr 8 10:34:43 CEST 2013


On Fri, 2013-04-05 at 19:00 +0200, Matthieu Bouron wrote:
> On Fri, Apr 05, 2013 at 11:52:14AM +0200, Tomas Härdin wrote:
> > On Thu, 2013-04-04 at 15:33 +0200, Matthieu Bouron wrote:
> > > On Thu, Apr 04, 2013 at 02:56:17PM +0200, Matthieu Bouron wrote:
> > > > On Thu, Apr 04, 2013 at 01:43:36PM +0200, Tomas Härdin wrote:
> > > > > On Fri, 2013-03-29 at 20:21 +0100, Matthieu Bouron wrote:
> > > > > > ---
> > > > > >  libavformat/mxfdec.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 131 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > > > > index 4580e1b..fa1a0fe 100644
> > > > > > --- a/libavformat/mxfdec.c
> > > > > > +++ b/libavformat/mxfdec.c
> > > > > > @@ -1619,6 +1619,136 @@ fail_and_free:
> > > > > >      return ret;
> > > > > >  }
> > > > > >  
> > > > > > +static int mxf_read_uid(AVIOContext *pb, int size, UID uid)
> > > > > > +{
> > > > > > +    if (size != 16)
> > > > > > +        return AVERROR(EINVAL);
> > > > > > +
> > > > > > +    avio_read(pb, uid, size);
> > > > > > +
> > > > > > +    return 0;
> > > > > > +}
> > > > > 
> > > > > Not required - just avio_read() in the macro. The spec mandates the size
> > > > > of this field, and accidental overreading on bad files is already
> > > > > handled.
> > > > > 
> > > > > > +static int mxf_read_utf16_string(AVIOContext *pb, int size, char** str)
> > > > > > +{
> > > > > > +    int ret;
> > > > > > +    char *buf;
> > > > > > +    if (size <= 0)
> > > > > > +        return AVERROR(EINVAL);
> > > > > 
> > > > > Strings can be empty, and size is never (?) negative.
> > > > 
> > > > Switched to strict comparison. size is never negative but since it is
> > > > used by av_mallocz and avio_read_str16be i kept the check.
> > > > 
> > > > > 
> > > > > > +
> > > > > > +    buf = av_malloc(size + 1);
> > > > > 
> > > > > *str = av_malloc() and drop buf altogether?
> > > > 
> > > > Done.
> > > > 
> > > > > 
> > > > > > +    if (!buf)
> > > > > > +        return AVERROR(ENOMEM);
> > > > > > +
> > > > > > +    if ((ret = avio_get_str16be(pb, size, buf, size + 1)) < 0) {
> > > > > 
> > > > > Size might need to be larger, or this might crap out certain strings.
> > > > 
> > > > buffer size should be ok since it is size + 1 large (twice the size needed
> > > > for the utf8 version of the string since avio_get_str16be performs a
> > > > convertion).
> > 
> > Isn't size the size of the UTF-16 data? There are cases where UTF-16 is
> > more efficient than UTF-8, meaning size isn't enough. From wikipedia
> > [1]:
> > 
> > > Characters U+0800 through U+FFFF use three bytes in UTF-8, but only
> > > two in UTF-16
> > 
> > So I think you should allocate size + size/2 + 1 bytes. Luckily the
> > solution you have right now shouldn't crash at least.
> > 
> > > New patch attached.
> > > Added missing do { } while (0) statement to macros (thanks to Clément).
> > 
> > Looks OK. One ultra-nit would be to sort the case labels in the switch.
> > 
> 
> Patch updated.

Looks good to me.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130408/e3fb34bf/attachment.asc>


More information about the ffmpeg-devel mailing list