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

[Tomas Härdin]

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.

/Tomas

[1] https://en.wikipedia.org/wiki/UTF-8#Disadvantages_4
-------------- 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/20130405/f546a1e8/attachment.asc>