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 753760 - rtph265: sync against latest spec
rtph265: sync against latest spec
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 753228
Blocks:
 
 
Reported: 2015-08-18 12:27 UTC by Luis de Bethencourt
Modified: 2016-07-07 12:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Cleanup of NALU type parsing and logging (2.35 KB, patch)
2016-06-17 11:09 UTC, Jonas Holmberg
committed Details | Review
Remove sprop-parameter-sets property (5.82 KB, patch)
2016-06-17 11:10 UTC, Jonas Holmberg
committed Details | Review
rtph265depay: Fix invalid read/write (valgrind) (2.21 KB, patch)
2016-06-30 13:11 UTC, Jonas Holmberg
none Details | Review
rtph265depay: Fix invalid read/write (valgrind) (2.33 KB, patch)
2016-06-30 13:17 UTC, Jonas Holmberg
committed Details | Review
rtph265pay/depay: Update to follow RFC 7798 (21.48 KB, patch)
2016-07-05 11:43 UTC, Jonas Holmberg
none Details | Review
rtph265pay/depay: Update to follow RFC 7798 (21.97 KB, patch)
2016-07-05 14:22 UTC, Jonas Holmberg
none Details | Review
rtph265pay/depay: Sync against RFC 7798 (22.54 KB, patch)
2016-07-06 10:43 UTC, Jonas Holmberg
none Details | Review
rtph265pay/depay: Sync against RFC 7798 (22.54 KB, patch)
2016-07-06 10:49 UTC, Jonas Holmberg
none Details | Review
rtph265pay/depay: Sync against RFC 7798 (22.55 KB, patch)
2016-07-07 11:48 UTC, Jonas Holmberg
committed Details | Review

Description Luis de Bethencourt 2015-08-18 12:27:52 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
Comment 1 Jonas Holmberg 2016-06-17 11:07:03 UTC
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...
Comment 2 Jonas Holmberg 2016-06-17 11:09:20 UTC
Created attachment 329943 [details] [review]
Cleanup of NALU type parsing and logging

Cosmetic changes only.
Comment 3 Jonas Holmberg 2016-06-17 11:10:34 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2016-06-17 12:26:57 UTC
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
Comment 5 Sebastian Dröge (slomo) 2016-06-17 12:28:10 UTC
(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 :)
Comment 6 Jonas Holmberg 2016-06-30 13:11:34 UTC
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.
Comment 7 Jonas Holmberg 2016-06-30 13:17:22 UTC
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 8 Tim-Philipp Müller 2016-06-30 16:03:39 UTC
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
Comment 9 Jonas Holmberg 2016-07-05 11:43:05 UTC
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.
Comment 10 Jonas Holmberg 2016-07-05 14:20:26 UTC
Opps, git hooks had been messed up so last patch wasn't gst-indented. Will upload a new patch...
Comment 11 Jonas Holmberg 2016-07-05 14:22:03 UTC
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.
Comment 12 Jonas Holmberg 2016-07-06 10:43:18 UTC
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.
Comment 13 Jonas Holmberg 2016-07-06 10:49:44 UTC
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.
Comment 14 Sebastian Dröge (slomo) 2016-07-07 07:39:04 UTC
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
Comment 15 Jonas Holmberg 2016-07-07 11:29:39 UTC
(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.
Comment 16 Sebastian Dröge (slomo) 2016-07-07 11:33:19 UTC
(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 :)
Comment 17 Jonas Holmberg 2016-07-07 11:46:51 UTC
(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.
Comment 18 Jonas Holmberg 2016-07-07 11:48:59 UTC
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.
Comment 19 Sebastian Dröge (slomo) 2016-07-07 12:03:33 UTC
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