[FFmpeg-devel] [PATCH] move avc sps/pps generation in a function in movenc

[Aurelien Jacobs]

Baptiste Coudurier wrote:

> Hi
> 
> Aurelien Jacobs wrote:
> > Hi,
> > 
> > The attached patch moves the avc sps/pps generation code in a
> > function. The final goal is to reuse this function from matroskaenc
> > (and so the next step will obviously be to move this func in its
> > own file). Is this patch OK ?
> > 
> > Aurel
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> > Index: libavformat/movenc.c
> > ===================================================================
> > --- libavformat/movenc.c	(revision 11447)
> > +++ libavformat/movenc.c	(working copy)
> > @@ -452,10 +452,10 @@
> >      return end + 3;
> >  }
> >  
> > -static int avc_parse_nal_units(uint8_t **buf, int *size)
> > +static int avc_parse_nal_units(uint8_t *buf_in, uint8_t **buf, int
> > *size)
> 
> Is changing the prototype really necessary ?

It could be avoided for movenc. But it's needed if you want the
source buffer to *not* be de-allocated in some cases (will be
required in matroskaenc).

> >  {
> >      ByteIOContext *pb;
> > -    uint8_t *p = *buf;
> > +    uint8_t *p = buf_in;
> >      uint8_t *end = p + *size;
> >      uint8_t *nal_start, *nal_end;
> >      int ret = url_open_dyn_buf(&pb);
> > @@ -475,22 +475,20 @@
> >      return 0;
> >  }
> >  
> > -static int mov_write_avcc_tag(ByteIOContext *pb, MOVTrack *track)
> > +static int avc_write_sps_pps(ByteIOContext *pb, uint8_t *data, int
> > len)
> 
> This specific formating was defined by iso and meant to be put in
> 'avcC' atom in iso media, loosing this information would be sad IMHO,
> I'd prefer something like isom_write_avcc.

And this specific formatting is also described in the matroska spec...
So I thought that describing what the function does instead of who
specified it, was more useful.
But if you insist, I will change the name. I don't care about it.

> > [...]
> >
> > +            int ret = avc_parse_nal_units(data, &buf, &len);
> > +            if (ret < 0)
> > +                return ret;
> 
> IMHO unrelated to the patch and there is another occurence which would
> need to be checked.

It is somewhat related, as this code is moved into a function returning
and int, and thus, it's good practice to return error codes.
But I will commit it separately. This should make things clearer.

> > -            avc_parse_nal_units(&track->vosData, &track->vosLen);
> > -            buf = track->vosData;
> > -            end = track->vosData + track->vosLen;
> > +            data = buf;
> 
> I don't think this last line is needed.

It is. See my next comment.

> >  [...]
> >  
> >              /* look for sps and pps */
> >              while (buf < end) {
> > @@ -522,10 +520,21 @@
> >              put_byte(pb, 1); /* number of pps */
> >              put_be16(pb, pps_size);
> >              put_buffer(pb, pps, pps_size);
> > +            av_free(data);
> 
> av_free(buf) should be more clear since buf was allocated, not data,
> data is parameter, this is confusing.

The problem is that buf is modified in the meantime, and thus, it
don't point to the start of the allocated memory anymore. That's
why a backup pointer to the start of allocated memory (data) is needed.

So, now, do you think the patch is OK, and do you want me to rename
the function to isom_write_avcc or not ?

Aurel