GNOME Bugzilla – Bug 775794
qtdemux: can not play xvid/mp2 quicktime format
Last modified: 2016-12-22 12:48:42 UTC
Created attachment 341600 [details] xvid/mp2 quicktime media file Quicktime(xvid/mp2) format media file that is encoded(trans-coded) by ffmpeg can not be played because decoder caps negotiation is failed.
Created attachment 341603 [details] [review] patch to enable xvid/mp2 (qtdemux.c) Video caps for xvid is video/mpeg version=4 but systemstream=false is omitted therefore caps negotiation is failed with mpeg4 video decoder. Even if systemstream=false is added in caps, quicktime/xvid video format that encoded by ffmpeg can not be played because qtdemux do not parse&handle glbl tag for xvid video. To fix it, 1. For XVID : Add systemstream=false in caps and parse&handle glbl node. 2. For mp2 : Add audio caps for mp2. 3. Extra : Add m2v1 fourcc to support mp2 video that is trans-coded by ffmpeg. I am attaching a patch for it.
Review of attachment 341603 [details] [review]: Generally looks good ::: gst/isomp4/qtdemux.c @@ +7230,3 @@ { const guint8 *buf; guint32 version; The code below looks rather... broken. First the FIXME comment, then it also misses length checks to see if it actually has enough data available to parse there (which looks both potentially security relevant). Maybe you can look into that in addition to this patch (as a separate patch)?
Created attachment 341669 [details] [review] patch : add to check length Thanks for the review. I am attaching one more patch including fixed code that you have mentioned. I am not sure about that it is right to display compressor name when the first bytes is wrong. But I think it might to be useful than ignore or return error. Because I think compressor name is not that important to return error. **The first bytes is the size in 14496-12, too.
Created attachment 341804 [details] [review] patch to check mp4 video sample description length I fixed more about FIXME and data length. I change data type int to guint32 for checking string length(compressor name).
Review of attachment 341603 [details] [review]: I would like to make it separately. I attached new patch for what you've mentioned. + if (node_length < 86) { + GST_WARNING_OBJECT (qtdemux, "%" GST_FOURCC_FORMAT + " sample description length too short (%u < 86)", + GST_FOURCC_ARGS (fourcc), node_length); + break; + } MP4 video sample description length should be bigger than 86, so I make it to stop parsing node if length is smaller than 86 (not return error and make new error type), because it is checking on parsing_track fucntion again. + str_len = QT_UINT8 (buf); + if (str_len < 32) + GST_DEBUG_OBJECT (qtdemux, "compressorname = %.*s", str_len, + (char *) buf + 1); + else + GST_WARNING_OBJECT (qtdemux, + "compressorname length too big (%u > 31)", str_len); + + buf += 32; /* the string has a reserved space of 32 bytes */ + buf += 4; /* 4 bytes reserved */ In my humble opinion, compressor name is not important (that filed is just for informative purposes on Spec.), therefore I added to check length and leave log when if it is wrong.
Review of attachment 341804 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +7235,3 @@ /* codec_data is contained inside these atoms, which all have * the same format. */ + if (node_length < 86) { Why 86? @@ +7248,1 @@ if (1 || version == 0x00000000) { This "if" here looks suspicious. That's always true :) @@ +7261,3 @@ + + buf += 32; /* the string has a reserved space of 32 bytes */ + buf += 4; /* 4 bytes reserved */ Compressor name is 32 bytes, that is: 1 byte length, up to 31 bytes string (not \0-terminated)
(In reply to Heekyoung Seo from comment #5) > Review of attachment 341603 [details] [review] [review]: > > I would like to make it separately. I attached new patch for what you've > mentioned. > > + if (node_length < 86) { > + GST_WARNING_OBJECT (qtdemux, "%" GST_FOURCC_FORMAT > + " sample description length too short (%u < 86)", > + GST_FOURCC_ARGS (fourcc), node_length); > + break; > + } > MP4 video sample description length should be bigger than 86, so I make it > to stop parsing node if length is smaller than 86 (not return error and make > new error type), because it is checking on parsing_track fucntion again. What about audio/others? Isn't that the same code? Also even if it is 86 at minimum, whenever we read more than 86 below we need to check if that much is available.
Review of attachment 341804 [details] [review]: 86 is the size of Video Sample Description. ::: gst/isomp4/qtdemux.c @@ +7235,3 @@ /* codec_data is contained inside these atoms, which all have * the same format. */ + if (node_length < 86) { 86 is the size of Video Sample Description in Specification. I will make new patch with mention about it. @@ +7248,1 @@ if (1 || version == 0x00000000) { Yes.. I saw it, too. :) I will fix it. @@ +7261,3 @@ + + buf += 32; /* the string has a reserved space of 32 bytes */ + GST_DEBUG_OBJECT (qtdemux, "compressorname = %.*s", str_len, Yes.. It is PASCAL string and reserved 32 bytes total . I mentioned it in upper comment.
Created attachment 341931 [details] [review] patch to check video sample description and XiTh length and XiTh What about audio/others? Isn't that the same code? > Yes, unless buffer length is smaller than node_length. Function will return false if buffer length is smaller than node length. Also even if it is 86 at minimum, whenever we read more than 86 below we need to check if that much is available. > constant value is used bellow. 86 > 51+31(it is read in below). Therefore, in my opinion, more checking is not needed. I am attaching new patch for it.
commit 35748dc8f23c11549ab79bb7571043a4294ec2ff Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Dec 14 10:15:10 2016 +0200 qtdemux: Check that the XiTh size is big enough https://bugzilla.gnome.org/show_bug.cgi?id=775794 commit f7c033f4ec8774ad194e8605388c597bee729e36 Author: Heekyoung Seo <heekyoung.seo@lge.com> Date: Fri Dec 9 20:27:53 2016 +0900 qtdemux: Check node length of video sample description Add check for node length of video sample description and its fields and for the XiTh atom. Also unify the code a bit. https://bugzilla.gnome.org/show_bug.cgi?id=775794 commit a5bfaf8a7964099716619e65315f9e4624d2d8c6 Author: Heekyoung Seo <heekyoung.seo@lge.com> Date: Thu Dec 8 18:50:52 2016 +0900 qtdemux: Enable xvid/mp2 codec support Add support for xvid video and mp2 audio, add m2v1 fourcc. https://bugzilla.gnome.org/show_bug.cgi?id=775794
Review of attachment 341603 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +13789,3 @@ "mpegversion", G_TYPE_INT, 1, NULL); break; + case GST_MAKE_FOURCC ('.', 'm', 'p', ' 2'): There's a typo here, fixed that before merging qtdemux.c: In function ‘qtdemux_audio_caps’: gstreamer/gst/gstvalue.h:45:74: error: result of ‘8242 << 24’ requires 39 bits to represent, but ‘int’ only has 32 bits [-Werror=shift-overflow=] #define GST_MAKE_FOURCC(a,b,c,d) ((guint32)((a)|(b)<<8|(c)<<16|(d)<<24)) ^ qtdemux.c:13775:10: note: in expansion of macro ‘GST_MAKE_FOURCC’ case GST_MAKE_FOURCC ('.', 'm', 'p', ' 2'): ^~~~~~~~~~~~~~~