[FFmpeg-devel] [PATCH] Matroska Muxer

[David Conrad]

On Aug 14, 2007, at 3:13 PM, Michael Niedermayer wrote:

> Hi
>
> On Mon, Aug 13, 2007 at 08:38:25PM -0400, David Conrad wrote:
>
>> Hi,
>>
>> I feel that my GSoC is ready for SVN. It does have a couple of known
>> limitations:
>>
>>
> [...]
>
>> +// 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
>>
>
> not doxygen compatible comment

Fixed, I think.

>> +
>> +// per-cuepoint-track - 3 1-byte EBML IDs, 3 1-byte EBML sizes, 2  
>> 8-byte uint max
>> +#define MAX_CUETRACKPOS_SIZE 22
>> +
>> +// per-cuepoint - 2 1-byte EBML IDs, 2 1-byte EBML sizes, 8-byte  
>> uint max
>> +#define MAX_CUEPOINT_SIZE(num_tracks) 12 +  
>> MAX_CUETRACKPOS_SIZE*num_tracks
>> +
>> +
>> +static int ebml_id_size(unsigned int id)
>> +{
>> +    return (av_log2(id+1)-1)/7+1;
>> +}
>> +
>> +static void put_ebml_id(ByteIOContext *pb, unsigned int id)
>> +{
>> +    int i = ebml_id_size(id);
>> +    while (i--)
>> +        put_byte(pb, id >> (i*8));
>> +}
>> +
>> +/**
>> + * Write an EBML size meaning "unknown size"
>> + *
>> + * @param bytes The number of bytes the size should occupy.  
>> Maximum of 8.
>> + */
>> +static void put_ebml_size_unknown(ByteIOContext *pb, int bytes)
>> +{
>> +    uint64_t value = 0;
>> +    int i;
>> +
>> +    bytes = FFMIN(bytes, 8);
>>
>
> what is this good for? >8 is not supported yes but it wont work any  
> better
> with such checks if its over 8

True, changed to an assert.

>> +    for (i = 0; i < bytes*7 + 1; i++)
>> +        value |= 1ULL << i;
>> +    for (i = bytes-1; i >= 0; i--)
>> +        put_byte(pb, value >> i*8);
>>
>
> put_byte(pb, 0x1FF>>bytes);
> while(--bytes)
>     put_byte(pb, 0xFF);

Fixed.

>> +}
>> +
>> +/**
>> + * Calculate how many bytes are needed to represent a given size  
>> in EBML
>> + */
>> +static int ebml_size_bytes(uint64_t size)
>> +{
>> +    int bytes = 1;
>> +    while ((size+1) >> bytes*7) bytes++;
>> +    return bytes;
>> +}
>>
>
> isnt ebml_size_bytes and ebml_id_size the same if the IDs would be  
> stored
> properly?
> i mean currently the #define *_ID_* is in encoded form while size  
> of course
> cannot be, so if we would change the #defines to be in normal form  
> then i
> think this could allow some simplifications, though i might be  
> wrong ...

I think this is possible, the only caveat is that if IDs with all  
ones are ever used (they're currently reserved) then this won't work  
for them. I'll send a patch changing the demuxer, then change the muxer.

>> +
>> +/**
>> + * Write a size in EBML variable length format.
>> + *
>> + * @param bytes The number of bytes that need to be used to write  
>> the size.
>> + *              If zero, any number of bytes can be used.
>> + */
>> +static void put_ebml_size(ByteIOContext *pb, uint64_t size, int  
>> bytes)
>> +{
>> +    int i, needed_bytes = ebml_size_bytes(size);
>> +
>>
>
>
>> +    // sizes larger than this are currently undefined in EBML
>> +    // so write "unknown" size
>> +    if (size >= (1ULL<<56)-1) {
>> +        put_ebml_size_unknown(pb, 1);
>> +        return;
>> +    }
>> +
>> +    if (bytes == 0)
>> +        // 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.
>>
>
> if it shoulnt happen then it should be an assert()

Fixed.

> [...]
>
>> + * @param size The number of bytes to reserve, which must be at  
>> least 2.
>> + */
>> +static void put_ebml_void(ByteIOContext *pb, uint64_t size)
>> +{
>> +    offset_t currentpos = url_ftell(pb);
>> +
>> +    if (size < 2)
>> +        return;
>>
>
> that should be an assert() and of course size must never be <2
> all checks which cant be true unless theres a bug somewhere in our  
> code
> should be assert()

I changed all of the checks that shouldn't happen to assert()s, I think.

> [...]
>
>> +static int mkv_add_seekhead_entry(mkv_seekhead *seekhead,  
>> unsigned int elementid, uint64_t filepos)
>> +{
>> +    mkv_seekhead_entry *entries = seekhead->entries;
>> +    int new_entry = seekhead->num_entries;
>> +
>> +    // don't store more elements than we reserved space for
>> +    if (seekhead->max_entries > 0 && seekhead->max_entries <=  
>> seekhead->num_entries)
>> +        return -1;
>> +
>> +    entries = av_realloc(entries, (seekhead->num_entries + 1) *  
>> sizeof(mkv_seekhead_entry));
>> +    if (entries == NULL)
>> +        return -1;
>>
>
> this should be AVERROR(ENOMEM)
> also not every call to this function checks the return value

Both fixed as well as for mkv_add_cuepoint().

>> +
>> +    entries[new_entry].elementid = elementid;
>> +    entries[new_entry].segmentpos = filepos - seekhead- 
>> >segment_offset;
>> +
>> +    seekhead->entries = entries;
>> +    seekhead->num_entries++;
>>
>
> the code mixes seekhead->num_entries and the local copy new_entry
>
> i would write
> entries[seekhead->num_entries  ].elementid  = elementid;
> entries[seekhead->num_entries++].segmentpos = filepos - seekhead- 
> >segment_offset;
>
> but using new_entry everywhere is fine too, just the mix of both is
> a little confusing

Okay, I agree that this is a bit clearer, so changed for both  
seekhead and cues.

> [...]
>
>
>> +                case CODEC_TYPE_SUBTITLE:
>> +                    put_ebml_uint(pb, MATROSKA_ID_TRACKTYPE,  
>> MATROSKA_TRACK_TYPE_SUBTITLE);
>> +                    break;
>> +            default:
>> +                av_log(s, AV_LOG_ERROR, "Only audio and video are  
>> supported for Matroska.");
>> +                break;
>>
>
> indention, and contradiction

Fixed.

> [...]
>
>> +    mkv->segment = start_ebml_master(pb, MATROSKA_ID_SEGMENT, 0);
>> +    mkv->segment_offset = url_ftell(pb);
>> +
>> +    // we write 2 seek heads - one at the end of the file to  
>> point to each cluster, and
>> +    // one at the beginning to point to all other level one  
>> elements (including the seek
>> +    // head at the end of the file), which isn't more than 10  
>> elements if we only write one
>> +    // of each other currently defined level 1 element
>> +    mkv->main_seekhead    = mkv_start_seekhead(pb, mkv- 
>> >segment_offset, 10);
>>
>
> i assume matroska needs seekable destination ...

It is possible to write matroska as a stream, but it's necessary to  
write lots of unknown sizes and I'm not sure how well parsers  
currently deal with that. At any rate, I think it should work now  
that seeks aren't required to write the extradata and other  
unnecessary seeks aren't done.

> [...]
>
>> +    mkv->duration = pkt->pts + pkt->duration;
>>
>
> FFMAX(mkv->duration, pkt->pts + pkt->duration);

Fixed.

New patch minus the combining of ebml_id_size/put_ebml_id with  
ebml_size_bytes/put_ebml_size attached.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mkvenc.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070820/d5421af7/attachment.txt>