GNOME Bugzilla – Bug 753760
rtph265: sync against latest spec
Last modified: 2016-07-07 12:03:33 UTC
Check if the RTP elements are still up to date with the latest spec draft: https://tools.ietf.org/html/draft-ietf-payload-rtp-h265-13
There is an rfc now: https://tools.ietf.org/html/rfc7798 and I am working on updating the elements. I will add patches to this ticket along the way...
Created attachment 329943 [details] [review] Cleanup of NALU type parsing and logging Cosmetic changes only.
Created attachment 329944 [details] [review] Remove sprop-parameter-sets property There is no valid use case when this property is needed since the values must be in either codec_data or buffer data.
commit 83ec89abdd23dd1606fa4e8bab949ce54299dec2 Author: Jonas Holmberg <jonashg@axis.com> Date: Fri Jun 17 13:00:48 2016 +0200 rtph265pay: Remove sprop-parameter-sets property There is no valid use case when this property is needed since the values must be in either codec_data or buffer data. https://bugzilla.gnome.org/show_bug.cgi?id=753760 commit 2039e0d88106a2d5d4f47c485834c49c312a6e6a Author: Jonas Holmberg <jonashg@axis.com> Date: Fri Jun 10 16:17:26 2016 +0200 rtph265pay: Read NALU type the same way everywhere Cosmetic change to read NALU type in gst_rtp_h265_pay_decode_nal() the same way as in other places. https://bugzilla.gnome.org/show_bug.cgi?id=753760
(In reply to Jonas Holmberg from comment #3) > Created attachment 329944 [details] [review] [review] > Remove sprop-parameter-sets property > > There is no valid use case when this property is needed since the values > must be in either codec_data or buffer data. Even more, it is not according to the spec. There is no sprop-parameter-set in h265 :)
Created attachment 330655 [details] [review] rtph265depay: Fix invalid read/write (valgrind) 10 bytes was allocated for stream_format but size of "byte-stream" is more. Use g_strdup() instead.
Created attachment 330656 [details] [review] rtph265depay: Fix invalid read/write (valgrind) 10 bytes was allocated for stream_format but size of "byte-stream" is more. Use g_strdup() instead.
Comment on attachment 330656 [details] [review] rtph265depay: Fix invalid read/write (valgrind) Thanks! commit 850a8bc0778880f9fe25f1e18276b9ef32d17c6f Author: Jonas Holmberg <jonashg@axis.com> Date: Thu Jun 30 15:01:46 2016 +0200 rtph265depay: fix invalid memory access 10 bytes was allocated for stream_format but size of "byte-stream" is more. Use g_strdup() instead. https://bugzilla.gnome.org/show_bug.cgi?id=753760
Created attachment 330899 [details] [review] rtph265pay/depay: Update to follow RFC 7798 Handle sprop-vps, sprop-sps and sprop-pps in caps instead of sprop-parameter-sets. rtph264pay works with byte-stream and hvc1 formats but not for hev1 yet and it handles profile-id, tier-flag and level-id in caps query.
Opps, git hooks had been messed up so last patch wasn't gst-indented. Will upload a new patch...
Created attachment 330907 [details] [review] rtph265pay/depay: Update to follow RFC 7798 Handle sprop-vps, sprop-sps and sprop-pps in caps instead of sprop-parameter-sets. rtph264pay works with byte-stream and hvc1 formats but not for hev1 yet and it handles profile-id, tier-flag and level-id in caps query.
Created attachment 330938 [details] [review] rtph265pay/depay: Sync against RFC 7798 Handle sprop-vps, sprop-sps and sprop-pps in caps instead of sprop-parameter-sets. rtph264pay works with byte-stream and hvc1 formats but not hev1 yet. It handles profile-id, tier-flag and level-id in caps query.
Created attachment 330940 [details] [review] rtph265pay/depay: Sync against RFC 7798 Handle sprop-vps, sprop-sps and sprop-pps in caps instead of sprop-parameter-sets. rtph265pay works with byte-stream and hvc1 formats but not hev1 yet. It handles profile-id, tier-flag and level-id in caps query.
Review of attachment 330940 [details] [review]: ::: gst/rtp/gstrtph265depay.c @@ +775,3 @@ + ps = NULL; + } else { + ps = g_strdup_printf ("%s,%s,%s", vps, sps, pps); It would probably be nicer to split this code apart so that each structure is parsed independently instead of concatenating them all together just to tear them apart again ::: gst/rtp/gstrtph265pay.c @@ +307,2 @@ + ptl[0] = value; + profile = gst_codec_utils_h265_get_profile (ptl, sizeof ptl); Parenthesis for sizeof @@ +404,3 @@ gst_buffer_unmap (vps_buf, &map); + g_string_append_printf (vps, "%s%s", i ? "," : "", set); You don't need GString for these anymore, the strings are always going to be exactly set
(In reply to Sebastian Dröge (slomo) from comment #14) > Review of attachment 330940 [details] [review] [review]: > > ::: gst/rtp/gstrtph265depay.c > @@ +775,3 @@ > + ps = NULL; > + } else { > + ps = g_strdup_printf ("%s,%s,%s", vps, sps, pps); > > It would probably be nicer to split this code apart so that each structure > is parsed independently instead of concatenating them all together just to > tear them apart again Yes, I agree. But I was thinking that it could be done in a later commit since it requires a bit of refactoring? > > ::: gst/rtp/gstrtph265pay.c > @@ +307,2 @@ > + ptl[0] = value; > + profile = gst_codec_utils_h265_get_profile (ptl, sizeof ptl); > > Parenthesis for sizeof OK. It's only needed when operand is a type name iirc, but there's no harm in having them. > > @@ +404,3 @@ > gst_buffer_unmap (vps_buf, &map); > > + g_string_append_printf (vps, "%s%s", i ? "," : "", set); > > You don't need GString for these anymore, the strings are always going to be > exactly set How do you mean? Each one of the three lists (vps, sps and pps) has to be comma separated.
(In reply to Jonas Holmberg from comment #15) > (In reply to Sebastian Dröge (slomo) from comment #14) > > Review of attachment 330940 [details] [review] [review] [review]: > > > > ::: gst/rtp/gstrtph265depay.c > > @@ +775,3 @@ > > + ps = NULL; > > + } else { > > + ps = g_strdup_printf ("%s,%s,%s", vps, sps, pps); > > > > It would probably be nicer to split this code apart so that each structure > > is parsed independently instead of concatenating them all together just to > > tear them apart again > > Yes, I agree. But I was thinking that it could be done in a later commit > since it requires a bit of refactoring? Probably not too much though? The first thing that happens a few lines below in all cases is "params = g_strsplit (ps, ",", 0);" You could just fill this params array instead of first creating the string > > @@ +404,3 @@ > > gst_buffer_unmap (vps_buf, &map); > > > > + g_string_append_printf (vps, "%s%s", i ? "," : "", set); > > > > You don't need GString for these anymore, the strings are always going to be > > exactly set > > How do you mean? Each one of the three lists (vps, sps and pps) has to be > comma separated. Ah I missed that there could be multiple VPS, PPS, SPS. Alright then :)
(In reply to Sebastian Dröge (slomo) from comment #16) > (In reply to Jonas Holmberg from comment #15) > > (In reply to Sebastian Dröge (slomo) from comment #14) > > > Review of attachment 330940 [details] [review] [review] [review] [review]: > > > > > > ::: gst/rtp/gstrtph265depay.c > > > @@ +775,3 @@ > > > + ps = NULL; > > > + } else { > > > + ps = g_strdup_printf ("%s,%s,%s", vps, sps, pps); > > > > > > It would probably be nicer to split this code apart so that each structure > > > is parsed independently instead of concatenating them all together just to > > > tear them apart again > > > > Yes, I agree. But I was thinking that it could be done in a later commit > > since it requires a bit of refactoring? > > Probably not too much though? The first thing that happens a few lines below > in all cases is "params = g_strsplit (ps, ",", 0);" > > You could just fill this params array instead of first creating the string Each of these (vps, sps and pps) can also be a comma separated list.
Created attachment 330992 [details] [review] rtph265pay/depay: Sync against RFC 7798 Handle sprop-vps, sprop-sps and sprop-pps in caps instead of sprop-parameter-sets. rtph265pay works with byte-stream and hvc1 formats but not hev1 yet. It handles profile-id, tier-flag and level-id in caps query.
commit a06152c40ac79f77ac9d275f00b40e4e9e3a3f81 Author: Jonas Holmberg <jonashg@axis.com> Date: Mon Jul 4 09:50:11 2016 +0200 rtph265pay/depay: Sync against RFC 7798 Handle sprop-vps, sprop-sps and sprop-pps in caps instead of sprop-parameter-sets. rtph265pay works with byte-stream and hvc1 formats but not hev1 yet. It handles profile-id, tier-flag and level-id in caps query. https://bugzilla.gnome.org/show_bug.cgi?id=753760