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 797279 - [regression] h265parser: Fix wrong maximum range check in gst_h265_parse_vps()
[regression] h265parser: Fix wrong maximum range check in gst_h265_parse_vps()
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.14.5
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-10-12 10:33 UTC by Seungha Yang
Modified: 2018-11-03 14:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
h265parser: Fix wrong maximum range check in gst_h265_parse_vps() (2.10 KB, patch)
2018-10-12 10:36 UTC, Seungha Yang
none Details | Review
[1/3] tests: h265parser: Add test parsing nonzero vps_max_layer_id in VPS (2.87 KB, patch)
2018-10-14 08:48 UTC, Seungha Yang
none Details | Review
[3/3] h265parser: Various update of vps parsing (3.92 KB, patch)
2018-10-14 08:52 UTC, Seungha Yang
none Details | Review

Description Seungha Yang 2018-10-12 10:33:59 UTC
I found regression after fix of bug #754124.
The patch in bug #754124 seems to correct but the actual bug is in h265parser
Comment 1 Seungha Yang 2018-10-12 10:36:46 UTC
Created attachment 373909 [details] [review]
h265parser: Fix wrong maximum range check in gst_h265_parse_vps()
Comment 2 Seungha Yang 2018-10-12 10:42:15 UTC
The error stream is private, it couldn't be shared, sorry. 

Spec. is saying that,
7.4.3.1 Video parameter set RBSP semantics
...

*vps_max_layer_id* specifies the maximum allowed value of nuh_layer_id of all NAL units in each CVS referring to the
VPS. vps_max_layer_id shall be less than 63 in bitstreams conforming to this version of this Specification. The value of 63
for vps_max_layer_id is reserved for future use by ITU-T | ISO/IEC. Although the value of vps_max_layer_id is required
to be less than 63 in this version of this Specification, decoders shall allow a value of vps_max_layer_id equal to 63 to
appear in the syntax.

*vps_num_layer_sets_minus1* plus 1 specifies the number of layer sets that are specified by the VPS. The value of
vps_num_layer_sets_minus1 shall be in the range of 0 to 1023, inclusive.

*vps_num_hrd_parameters* specifies the number of hrd_parameters( ) syntax structures present in the VPS RBSP before
the vps_extension_flag syntax element. The value of vps_num_hrd_parameters shall be in the range of 0 to
vps_num_layer_sets_minus1 + 1, inclusive.

*hrd_layer_set_idx[ i ]* specifies the index, into the list of layer sets specified by the VPS, of the layer set to which the i-th
hrd_parameters( ) syntax structure in the VPS applies. The value of hrd_layer_set_idx[ i ] shall be in the range of
( vps_base_layer_internal_flag ? 0 : 1 ) to vps_num_layer_sets_minus1, inclusive.
Comment 3 Nicolas Dufresne (ndufresne) 2018-10-12 18:38:06 UTC
I haven't checked the code yet, you could extract the headers NAL from the stream (or craft one), and wrap this in a unit test. I don't think the NAL along can be considered a "copy". I'm looking forward encouraging more unit test for parsers in general.
Comment 4 Seungha Yang 2018-10-14 08:48:22 UTC
Created attachment 373923 [details] [review]
[1/3] tests: h265parser: Add test parsing nonzero vps_max_layer_id in VPS

Add unit test with extracted VPS
Comment 5 Seungha Yang 2018-10-14 08:52:52 UTC
Created attachment 373924 [details] [review]
[3/3] h265parser: Various update of vps parsing

* Add FIXME for future correction of HRDParames parsing.
Spec. defines that the number of HRDParames could be up to
"vps_num_layer_sets_minus1 + 1" (i.e., 1024).

* Add parsing vps_base_layer_{internal,available}_flag.

* Fix possible invalid vps_extension parsing.

I'm not sure whether this patch can be applied to stable branch or not because
the GstH265VPS structure is modified. 
So split this from previous uploaded one.
Comment 6 Edward Hervey 2018-10-22 06:22:04 UTC
Is the vps_num_layer_sets related to L-HEVC (Scalable codecs) ?
Comment 7 Seungha Yang 2018-10-22 06:45:22 UTC
(In reply to Edward Hervey from comment #6)
> Is the vps_num_layer_sets related to L-HEVC (Scalable codecs) ?

Yes. vps_num_layer_sets specifies the number of layer set in the stream. The layer here can be 3D (left, right) layer or temporal or spatial (base/enhancement) layer if my understanding is correct.
Comment 8 GStreamer system administrator 2018-11-03 14:35:45 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/798.