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 366155 - [matroskademux] Several problems in encoding handling code
[matroskademux] Several problems in encoding handling code
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 0.10.5
Assigned To: Tim-Philipp Müller
GStreamer Maintainers
Depends on:
Reported: 2006-10-28 08:15 UTC by Michal Benes
Modified: 2006-10-30 16:39 UTC
See Also:
GNOME target: ---
GNOME version: ---

Fix several decoding problems (3.70 KB, patch)
2006-10-28 08:23 UTC, Michal Benes
committed Details | Review

Description Michal Benes 2006-10-28 08:15:47 UTC
There are some problems with the recent patch that adds support for track encoding

In gst_matroska_decode_buffer it is assumed that encoding type is compression. And therefore possibly uninitialized value enc.comp_algo is used. Matroska demuxer therefore tries to unzip encrypted tracks.

Not all possible values of comp_algo are checked (comp_algo == 3 is missing), therefore new_data may stay NULL and buffer referencing NULL data may be pushed to sink pad.

There is a for-cycle over all encodings, but new_data is generated allways from the original buffer (and old new_data leaked).

Data should be decoded according to encoding order (I have sorted encodings decreasingly in enc.order in my patch)

I have also removed \n from GST_WARNINGS

(patch follows)
Comment 1 Michal Benes 2006-10-28 08:23:20 UTC
Created attachment 75556 [details] [review]
Fix several decoding problems

In this patch, I do not print any warning that data decryption is not supported. In fact, EncodingType == 1 does not imply that the data are encrypted and can not be played as such.

Also, I do not like printing warning for every buffer. It would be probably nicer to write the decryption warning only once. I would gladly provide a patch for this if you agree that it is a good idea.
Comment 2 Tim-Philipp Müller 2006-10-30 16:39:54 UTC
Looks good, thanks.

 2006-10-30  Tim-Philipp Müller  <tim at centricular dot net>

        Patch by: Michal Benes  <michal dot benes at itonis tv>

        * gst/matroska/matroska-demux.c: (gst_matroska_demux_encoding_cmp),
          Fix several issues with encoded/compressed/encrypted/signed tracks;
          also, remove superfluous newline characters from some debug
          statements. (#366155)

I'm not entirely sure what to do about buffers we can't decode or decompress. I am not really sure whether it's a good idea to push them as they are (as opposed to just dropping them).

We might also want some check in place to ensure that there is at least one track we can decode and throw an error if not.

It seems that according to the spec EncodingType == 1 && ContentEncAlgo == 0 means that the buffers are signed rather than encrypted. Is this what you meant or are there more cases?

Not printing the warning for every buffer would indeed be a good idea. Care to make a patch for that too? :)

Btw, if you happen to have some (not so big) sample files that use track encodings/compressions/encryptions, I'd be quite interested to get my hands on them.