[FFmpeg-devel] [PATCH] Matroska Muxer

[Michael Niedermayer]

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. Stream copy from raw flac doesn't work since the demuxer passes the 
> extradata in the first packet. I think it would be preferable to change the 
> demuxer?

implement AVCodecParser.split() for flac


[...]
> As for applying this, I plan on using git-svn to push the history of 
> matroskaenc.c, then apply the rest of the patch to enable it in one go. 
> Does that sound okay?

yes commiting the whole history is preferred


>
> Comments and more testing welcome.

[...]

> +// 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


> +
> +// 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


> +    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);


> +}
> +
> +/**
> + * 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 ...


> +
> +/**
> + * 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()


[...]
> + * @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()


[...]
> +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


> +
> +    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


[...]

> +                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


[...]
> +    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 ...


[...]
> +    mkv->duration = pkt->pts + pkt->duration;

FFMAX(mkv->duration, pkt->pts + pkt->duration);


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070814/d7d0a337/attachment.pgp>