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

[David Conrad]

On May 3, 2010, at 3:20 PM, Baptiste Coudurier wrote:

> On 05/03/2010 03:50 AM, Martin Storsj? wrote:
>> 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
> 
> Yes, both patches are good.

Applied.