Created attachment 53873 [details] [review]
Patch adding SimpleBlock support to both Matroska muxer and demuxer.

New parameter version is introduced to matroskamux. This determines whether
matroska v1 or matroska v2 (with SimpleBlocks) should be used.

Patch looks very good to me.

Just one question:

In matroska_mux_write_data() you add this:

+  /* write the block, for matroska v2 use SimpleBlock if possible
+   * one slice (*breath*).
+   * FIXME: lacing, etc. */

What's the implication of the 'FIXME: lacing etc.'? How important is that for a
functional muxer and valid output? (And what does the '*breath*' mean? Like
'*gasp*'?)


I also wondered about this:

  *(guint16 *) & GST_BUFFER_DATA (hdr)[1] = foo;

Aren't there alignment issues with that on some platforms? Maybe it would be
better to use GST_WRITE_UINT16_LE, GST_WRITE_UINT16_BE and friends here. (I know
this was in the original code, I'm just wondering).

 Cheers
  -Tim

This *breath* comment was in the original code too. Lacing mechanism allows
storing more frames into one block. This reduces the container overhead,
especially when the frames are small. It would be nice to have lacing, but I do
not really need it at the moment, so I have not implemented it.

Yes, you are right, if these convenient macros exists, they should be used.
Please, could you do that? I am a bit lazy to resend the patch because of one
line ;)

Cheers
  Michal

Yeah, of course. Thanks a lot for the patch, it's in CVS now:


2005-10-28  Michal Benes  <michal dot benes at xeris dot cz>

        * gst/matroska/matroska-demux.c: (gst_matroska_demux_init_stream),
        (gst_matroska_demux_parse_info),
        (gst_matroska_demux_parse_blockgroup_or_simpleblock),
        (gst_matroska_demux_parse_cluster):
        * gst/matroska/matroska-ids.h:
        * gst/matroska/matroska-mux.c: (gst_matroska_mux_class_init),
        (gst_matroska_mux_init), (gst_matroska_mux_start),
        (gst_matroska_mux_create_buffer_header),
        (gst_matroska_mux_write_data), (gst_matroska_mux_set_property),
        (gst_matroska_mux_get_property):
        * gst/matroska/matroska-mux.h:
          Add SimpleBlock support to matroska demuxer and muxer (part of
          Matroska v2). (#319731)

 Cheers
  -Tim

*breath* = *gasp*, I wrote that after writing the original muxer and looking at
the specs for a few too many days.