[FFmpeg-devel] [PATCH 4/4] movenc: Write QuickTime chapters

[Martin Storsjö]

On Mon, 26 Apr 2010, Martin Storsj? wrote:

> On Wed, 21 Apr 2010, David Conrad wrote:
> 
> > On Apr 19, 2010, at 5:27 PM, Baptiste Coudurier wrote:
> > 
> > > Hi David,
> > > 
> > > On 04/19/2010 10:24 AM, David Conrad wrote:
> > >> ---
> > >>  libavformat/movenc.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++---
> > >>  1 files changed, 73 insertions(+), 5 deletions(-)
> > >> 
> > >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > >> index 4efed1b..3cec73c 100644
> > >> --- a/libavformat/movenc.c
> > >> +++ b/libavformat/movenc.c
> > >> @@ -77,12 +77,14 @@ typedef struct MOVIndex {
> > >>      MOVIentry   *cluster;
> > >>      int         audio_vbr;
> > >>      int         height; ///<  active picture (w/o VBI) height for D-10/IMX
> > >> +    int         qt_chapterid; ///<  trackID of the qt chapter track
> > > 
> > > Any reason why putting qt_ in front of the name ?
> > 
> > I thought it might be confusing since there's two different chapter methods, changed to use a more generic tref naming.
> > 
> > >>  } MOVTrack;
> > >> 
> > >>  typedef struct MOVMuxContext {
> > >>      int     mode;
> > >>      int64_t time;
> > >>      int     nb_streams;
> > >> +    int     qt_chapters; ///<  number of the qt chapter track
> > > 
> > > Ditto.
> > > 
> > > > [...]
> > >> 
> > >> +static int mov_write_packet(AVFormatContext *s, AVPacket *pkt);
> > > 
> > > Can the function be moved to avoid forward declaration ?
> > > If yes, please move it.
> > 
> > 
> > Done
> > 
> > >> +
> > >> +// QuickTime chapters involve an additional text track with the chapter names
> > >> +// as samples, and a tref pointing from the other tracks to the chapter one.
> > >> +static void mov_create_qt_chapter_track(AVFormatContext *s, int tracknum)
> > > 
> > > _qt_ seems useless to me. there is already mov_ prefix.
> > > 
> > >> +{
> > >> +    MOVMuxContext *mov = s->priv_data;
> > >> +    MOVTrack *track =&mov->tracks[tracknum];
> > >> +    AVPacket pkt = { .stream_index = tracknum, .flags = AV_PKT_FLAG_KEY };
> > >> +    int i, len;
> > >> +
> > >> +    track->mode = mov->mode;
> > >> +    track->tag = MKTAG('t','e','x','t');
> > >> +    track->timescale = MOV_TIMESCALE;
> > >> +    track->enc = avcodec_alloc_context();
> > > 
> > > Shouldn't enc be freed ?
> > 
> > It was after writing trak, but I guess it makes more sense to do so in write_trailer
> > 
> > > > [...]
> > > >
> > >> @@ -1832,7 +1894,11 @@ static int mov_write_header(AVFormatContext *s)
> > >>          }
> > >>      }
> > >> 
> > >> -    mov->tracks = av_mallocz(s->nb_streams*sizeof(*mov->tracks));
> > >> +    mov->nb_streams = s->nb_streams;
> > >> +    if (mov->mode&  (MODE_MOV|MODE_IPOD)&&  mov->nb_streams&&  s->nb_chapters)
> > >> +        mov->qt_chapters = mov->nb_streams++;
> > > 
> > > mov->nb_streams cannot be 0 since s->nb_streams cannot.
> > 
> > Fixed
> 
> Baptiste, is this ok to be commited? I've based my RTP hinting patches on 
> this now, since it includes some generic things that I need, too.

Ping

// Martin