[FFmpeg-devel] [PATCH] Matroska Muxer

[David Conrad]

On Aug 14, 2007, at 5:36 AM, Diego Biurrun wrote:

> On Mon, Aug 13, 2007 at 08:38:25PM -0400, David Conrad wrote:
>
>>
>> Comments and more testing welcome.
>>
>
> Some nits below, the build system changes are OK.
>
>
>> --- /dev/null
>> +++ b/libavformat/matroskaenc.c
>> @@ -0,0 +1,811 @@
>> +/*
>> + * Matroska file muxer
>>
>
> I think just Matroska muxer is enough.
>
>
>> +// 2 bytes * 3 for EBML IDs, 3 1-byte EBML lengths, 8 bytes for  
>> 64 bit offset, 4 bytes for target EBML ID
>> +#define MAX_SEEKENTRY_SIZE 21
>> +
>> +// per-cuepoint-track - 3 1-byte EBML IDs, 3 1-byte EBML sizes, 2  
>> 8-byte uint max
>> +#define MAX_CUETRACKPOS_SIZE 22
>>
>
> C has multiline comments :)  Please keep lines below 80 characters  
> where
> it makes sense.
>
>
>> +/**
>> + * Calculate how many bytes are needed to represent a given size  
>> in EBML
>>
>
> Add a period at the end.
>
>
>> +        // don't care how many bytes are used, so use the min
>> +        bytes = needed_bytes;
>> +    else if (needed_bytes > bytes) {
>> +        // the bytes needed to write the given size would exceed  
>> the bytes
>> +        // that we need to use, so write unknown size. This  
>> shouldn't happen.
>>
>
>
>> +/**
>> + * Writes a void element of a given size. Useful for reserving  
>> space in the file to be
>> + * written to later.
>>
>
> needlessly long line
>
>
>> +    put_ebml_id(pb, EBML_ID_VOID);
>> +    // we need to subtract the length needed to store the size  
>> from the size we need to reserve
>> +    // so 2 cases, we use 8 bytes to store the size if possible,  
>> 1 byte otherwise
>>
>
> ditto
>
>
>> +/**
>> + * Initialize a mkv_seekhead element to be ready to index level 1  
>> Matroska elements.
>> + * If a maximum number of elements is specified, enough space  
>> will be reserved at
>> + * the current file location to write a seek head of that size.
>> + *
>> + * @param segment_offset The absolute offset to the position in  
>> the file where the segment begins
>> + * @param numelements the maximum number of elements that will be  
>> indexed by this
>> + *                    seek head, 0 if unlimited.
>>
>
> again
>
>
>> +        // 21 bytes max for a seek entry, 10 bytes max for the  
>> SeekHead ID and size,
>> +        // and 3 bytes to guarantee that an EBML void element  
>> will fit afterwards
>>
>
> and again
>
>
>> +/**
>> + * Write the seek head to the file and free it. If a maximum  
>> number of elements was
>> + * specified to mkv_start_seekhead(), the seek head will be  
>> written at the location
>> + * reserved for it. Otherwise, it is written at the current  
>> location in the file.
>>
>
> and ..
>
> .. some more occurrences can be found below ..
>
>
>> +        av_log(codec, AV_LOG_WARNING, "no aac extradata, unable  
>> to determine sample rate\n");
>>
>
> AAC, samplerate, same below

All these should be fixed now.