GNOME Bugzilla – Bug 787206
matroska-demux: incorrectly interprets FlagInterlaced field
Last modified: 2018-08-23 21:38:24 UTC
matroska-demux.c interprets the FlagInterlaced-field as if it was a boolean value, interpreting absence and a value of 0 as "progressive", while other values are interprered as "interlaced": https://github.com/GStreamer/gst-plugins-good/blob/286df32e038c803fa40e89d48a7fc3e53f899601/gst/matroska/matroska-demux.c#L676 This is contrary to the specification, which states > FlagInterlaced A flag to declare is the video is known to be progressive or interlaced and if applicable to declare details about the interlacement. (0: undetermined, 1: interlaced, 2: progressive) https://www.matroska.org/technical/specs/index.html#FlagInterlaced I prepared Matroska-Fiels with the corresponding values in the interlaced field: https://c3voc.mazdermind.de/permanent/voctomix-issue-137-ffmpeg-interlaced-mkv/all-mode-mkvs.zip They can be analyzed using `mkvinfo file.mkv | grep -iA5 "+ Video track"` for their actual header value. The following code can be used to test gstreames interpretation of them (tested on git master): > TESTFILES="no-interlaced-field.mkv interlaced-0.mkv interlaced-1.mkv interlaced-2.mkv" > > for file in $TESTFILES; do > echo $file > GST_DEBUG=matroskademux:4 gst-launch-1.0 -q filesrc location=$file ! matroskademux ! video/x-raw ! fakesink 2>&1 | grep interlace-mode > echo "" > done Which produces (shortend) > no-interlaced-field.mkv > video/x-raw, format=(string)I420, width=(int)1920, height=(int)1080, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1, chroma-site=(string)mpeg2, colorimetry=(string)bt709, framerate=(fraction)25/1 > > interlaced-0.mkv > video/x-raw, format=(string)I420, width=(int)1920, height=(int)1080, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1, chroma-site=(string)mpeg2, colorimetry=(string)bt709, framerate=(fraction)25/1 > > interlaced-1.mkv > video/x-raw, format=(string)I420, width=(int)1920, height=(int)1080, interlace-mode=(string)mixed, pixel-aspect-ratio=(fraction)1/1, chroma-site=(string)mpeg2, colorimetry=(string)bt709, framerate=(fraction)25/1 > > interlaced-2.mkv > video/x-raw, format=(string)I420, width=(int)1920, height=(int)1080, interlace-mode=(string)mixed, pixel-aspect-ratio=(fraction)1/1, chroma-site=(string)mpeg2, colorimetry=(string)bt709, framerate=(fraction)25/1 More information regarding ffmpeg's handling of this field can be read in the original bug report against voctomix: https://github.com/voc/voctomix/issues/137
Do you want to provide a patch for this? The muxer probably also has to be updated
@slomo I initially was looking for a peer review, ensuring that I have not missed something obvious. In case you don't see an obvious miss I could try try to come up with a patch, but it may take its time and I would not mind if it is has already been done then.
Created attachment 359330 [details] [review] matroskademux: FlagInterlaced is an enum, not a flag 0 means unspecified, 1 means interlaced, 2 means progressive. If we handle it like a flag, all progressive content is considered interlaced.
How about this patch?
This patch handles the cases of GST_MATROSKA_ID_VIDEOFLAGINTERLACED==1 (mapped to interlaced) GST_MATROSKA_ID_VIDEOFLAGINTERLACED==2 (mapped to progressive) It would also keep the mapping of an absent GST_MATROSKA_ID_VIDEOFLAGINTERLACED-field to progressive which is questionable but probably ok from an compatibility point of view. Problematic is the case of GST_MATROSKA_ID_VIDEOFLAGINTERLACED==0 (== undetermined) which would be mapped to progressive. As far as I can see gstreamer has the notion of mixed-content (interlace-mode=mixed) versus interlace-mode=progressive/interlaced and a value of GST_MATROSKA_ID_VIDEOFLAGINTERLACED==0 should probably be mapped to interlace-mode=mixed. Furthermore setting GST_MATROSKA_VIDEOTRACK_INTERLACED-flag seems to currently trigger interlace-mode=mixed (see output of gst-launch-1.0 commands above) This is also incorrect.
Mapping 0 to mixed seems as wrong as mapping it to anything else. Or is there something in the Matroska specs about what it exactly is supposed to mean?
So what should happen here?
Your argumentations seems correct. Following it I'd propose the following mapping: <absent> -> no interlace-mode caps 0 -> no interlace-mode caps 1 -> interlace-mode=interlaced 2 -> interlace-mode=progressive This would require adding another flag or, better, an enum to signal the no-information state.
Without the field, it is implicitly progressive though
What should we do about this?
> Your argumentations seems correct. Following it I'd propose the following > mapping: > > <absent> -> no interlace-mode caps > 0 -> no interlace-mode caps > 1 -> interlace-mode=interlaced > 2 -> interlace-mode=progressive > > This would require adding another flag or, better, an enum to signal the > no-information state. Only saw your patch after I had already implemented this Sebastian, sorry! I think we should not make up information, so even if it later gets implicitly assumed to be progressive we shouldn't put progressive mode into the demuxer caps unless we know, IMHO: Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Aug 23 22:57:35 2018 +0200 matroska: fix handling of FlagInterlaced This is an enum not a boolean, and a value of 2 signals that the video is progressive, but we would mistakenly set interlace-mode=mixed on the output caps. https://bugzilla.gnome.org/show_bug.cgi?id=787206