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 765365 - h264parse: Fails to parse HRD parameters in SPS that ffmpeg accepts just fine
h264parse: Fails to parse HRD parameters in SPS that ffmpeg accepts just fine
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-21 07:36 UTC by Nicola
Modified: 2018-11-03 13:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
annotations (1.29 KB, patch)
2016-04-21 08:05 UTC, Sebastian Dröge (slomo)
reviewed Details | Review

Description Nicola 2016-04-21 07:36:14 UTC
Please try this pipeline using the provided public url

gst-launch-1.0 -vmt rtspsrc location="rtspt://admin:q1w2e3r4t5y6@93.63.189.11:45433/onvif/profile5/media.smp" ! rtph264depay ! h264parse  ! matroskamux ! fakesink silent=false

you'll get a not negotiated error from rtspsrc

if you remove matroskamux h264parse probably works in passthrough mode and the stream can be played
Comment 1 Sebastian Dröge (slomo) 2016-04-21 07:52:05 UTC
Problem is (at least) that h264parse does not put width/height into the caps, matroskamux needs them.
Comment 2 Sebastian Dröge (slomo) 2016-04-21 08:05:49 UTC
Reasons is that parsing the SPS fails. The VUI has invalid HRD.

hrd->cpb_cnt_minus1 is 1, so 2 are parsed. But the second fails right in the beginning in gst_h264_parse_hrd_parameters(). Changing that loop to < instead of <= makes it work.
Comment 3 Sebastian Dröge (slomo) 2016-04-21 08:05:59 UTC
Created attachment 326474 [details] [review]
annotations
Comment 4 Sebastian Dröge (slomo) 2016-04-21 08:10:11 UTC
Sree, can you take a look at those two parts of the code and compare it to the spec?
Comment 5 sreerenj 2016-04-22 08:42:20 UTC
Comment on attachment 326474 [details] [review]
annotations


> 
>+  /* FIXME Works if making this < instead of <= */
>   for (sched_sel_idx = 0; sched_sel_idx <= hrd->cpb_cnt_minus1; sched_sel_idx++) {

No, the existing code is right: as per spec
for( SchedSelIdx = 0; SchedSelIdx <= cpb_cnt_minus1; SchedSelIdx++ ) {
Comment 6 sreerenj 2016-04-22 08:44:42 UTC
Review of attachment 326474 [details] [review]:

::: gst-libs/gst/codecparsers/gsth264parser.c
@@ +1950,3 @@
       gint i;
 
+      /* FIXME Shouldn't this be <= ? */

As per Spec,

if( slice_group_map_type = = 2 )
for( iGroup = 0; iGroup < num_slice_groups_minus1; iGroup++ )
Comment 7 Sebastian Dröge (slomo) 2016-04-22 09:08:00 UTC
Ok, so it's all behaving according to the spec. Maybe we should ignore parsing errors in that code then?
Comment 8 sreerenj 2016-04-22 09:22:50 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Ok, so it's all behaving according to the spec. Maybe we should ignore
> parsing errors in that code then?

Hm, we have more issues, vaapidecode will fail since it is also parsing headers..
Comment 9 Sebastian Dröge (slomo) 2016-04-26 08:11:22 UTC
As ffmpeg can handle the stream just fine, it should somehow be possible to ignore the problems :)
Comment 10 Nicola 2016-04-28 10:09:33 UTC
the macro READ_UE and READ_UINT8 have a goto error if parsing fails, maybe we can create a new macro without the goto or use directly nal_reader_get_ue/nal_reader_get_bits_uint8 inside the for, ignoring the errors make the parser works properly, something like this (note the additional read):

for (sched_sel_idx = 0; sched_sel_idx <= hrd->cpb_cnt_minus1; sched_sel_idx++) {
    //READ_UE (nr, hrd->bit_rate_value_minus1[sched_sel_idx]);
    nal_reader_get_ue (nr, &hrd->bit_rate_value_minus1[sched_sel_idx]);
    //READ_UE (nr, hrd->cpb_size_value_minus1[sched_sel_idx]);
    nal_reader_get_ue (nr, &hrd->cpb_size_value_minus1[sched_sel_idx]);
    READ_UINT8 (nr, hrd->cbr_flag[sched_sel_idx], 1);
    nal_reader_get_bits_uint8 (nr, &hrd->cbr_flag[sched_sel_idx], 1);
  }

are these fields optional?

For your info I reported the issue to the camera manufacter too
Comment 11 Sebastian Dröge (slomo) 2016-04-28 10:13:35 UTC
They are "optional" as in there is a flag elsewhere that specifies if they are there or not. The flag is set and then they must be there.
Comment 12 sreerenj 2016-04-28 11:11:45 UTC
HRD parameters are the part of sps headers. We have to handle it carefully. May be a field in sps to indicate the vui parsing isn't success and also generate the warning when it failed to parse VUI. Because we have to make sure sps parsing returns TRUE , but never use any vui related parameters since it wrongly encoded.

Or an ugly hack by setting vui_parameters_present_flag = FALSE if any failure in vui parsing.
Comment 13 GStreamer system administrator 2018-11-03 13:49:40 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/379.