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 749655 - vtdec: Require width and height field for H264
vtdec: Require width and height field for H264
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2015-05-20 19:06 UTC by Nicolas Dufresne (ndufresne)
Modified: 2015-06-01 15:27 UTC
See Also:
GNOME target: ---
GNOME version: ---

vtdec: Require width and height field for H264 (1.06 KB, patch)
2015-05-20 19:06 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2015-05-20 19:06:41 UTC
This decoder does not work if width and height field are not set
in the sinkpad caps. Let's make this explicit by adding them to
the template caps.
Comment 1 Nicolas Dufresne (ndufresne) 2015-05-20 19:06:45 UTC
Created attachment 303694 [details] [review]
vtdec: Require width and height field for H264
Comment 2 Nicolas Dufresne (ndufresne) 2015-05-20 19:08:21 UTC
Note, this patch is not tested, just wrote it while debugging with samskiter on IRC. Review attentively.
Comment 3 Sebastian Dröge (slomo) 2015-05-21 09:08:17 UTC
Comment on attachment 303694 [details] [review]
vtdec: Require width and height field for H264

Question is also why h264parse did not add the width/height to the caps for him...
Comment 4 Nicolas Dufresne (ndufresne) 2015-05-24 02:56:33 UTC
Normally you don't need h264parse when using RTP depayloader. Though, I think I remember now that width and height parsing was only added in master recently (that bug was reported against 1.4). What was my thinking around proposing this patch, is that if we strictly require width and height, it's probably better to expose it in the template caps, making it fail earlier. Does that make sense ?

(Adding h264parse after the depayloader indeed fixed the issue)
Comment 5 Sebastian Dröge (slomo) 2015-05-24 12:50:47 UTC
Of course, the patch is good and should be merged :) I'm just wondering if there is also another bug hiding here in h264parse (or rtph264depay): not exposing width/height sometimes. We fixed some things related to that since 1.4, but maybe more is left?
Comment 6 Nicolas Dufresne (ndufresne) 2015-05-24 15:52:09 UTC
Ok, I've looked in master, and interestingly Wim reverted that work. As an example, for the hypothetical pipeline:

  someh264src ! rtph264pay ! rtph264depay ! vtdec ! ...

The caps received by vtdec will not contain width and height, forcing a parser. Note that adding a parser works. After a discussion, Olivier started adding width and height parsing (from sps/pps), so they would be present in the depayloaded caps. But might have been mistakenly added to the payloaded caps too, that would explain the revert.

Having width and height in the depayloaded caps (read from SPS/PPS), was the only way we found not to have to parse twice the H264 stream. From Olivier, it seemed that it was wanted that (de)payloaders should not require a parser (for performance reason).
Comment 7 Sebastian Dröge (slomo) 2015-05-24 16:09:55 UTC
Yes the depayloader should put it on the caps if it can. Note that decodebin will still autoplug parsers in any case.
Comment 8 Nicolas Dufresne (ndufresne) 2015-06-01 15:24:07 UTC
Attachment 303694 [details] pushed as 4cca6ef - vtdec: Require width and height field for H264
Comment 9 Nicolas Dufresne (ndufresne) 2015-06-01 15:27:33 UTC
There is a separate bug that suggest adding PPS/SPS parsing to the depayloader in order to better avoid the need for an extra parser. This would simply be an optimization. I think Farstream may not work with vtdec at the moment.