After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 319731 - [matroska] SimpleBlock support for muxer and demuxer
[matroska] SimpleBlock support for muxer and demuxer
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.9.x
Other Linux
: Normal enhancement
: 0.9.5
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-10-25 13:16 UTC by Michal Benes
Modified: 2005-10-28 15:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch adding SimpleBlock support to both Matroska muxer and demuxer. (10.62 KB, patch)
2005-10-25 13:19 UTC, Michal Benes
committed Details | Review

Description Michal Benes 2005-10-25 13:16:34 UTC
This patch add SimpleBlock support to both Matroska muxer and demuxer.
Comment 1 Michal Benes 2005-10-25 13:19:05 UTC
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.
Comment 2 Tim-Philipp Müller 2005-10-28 08:52:28 UTC
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
Comment 3 Michal Benes 2005-10-28 14:37:32 UTC
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
Comment 4 Tim-Philipp Müller 2005-10-28 15:33:59 UTC
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
Comment 5 Ronald Bultje 2005-10-28 15:50:49 UTC
*breath* = *gasp*, I wrote that after writing the original muxer and looking at
the specs for a few too many days.