GNOME Bugzilla – Bug 769717
h265parser: refactoring some methods to be more like h264parser
Last modified: 2018-11-03 13:54:17 UTC
I think we should refactor h265parser's methods to be more like h264parser for example gst_h265_parse_process_nal, gst_h265_parse_handle_frame in gst_h265_parse_handle_frame there is part with "no SPS/PPS yet, nal Type: %d %s, Size: %u will be dropped" that is really not good h264parser don't have something like this
Created attachment 333081 [details] [review] h265parse handle_frame and parse_process_nal more like in h264parse
Review of attachment 333081 [details] [review]: Generally looks good but seems incomplete: ::: gst/videoparsers/gsth265parse.c @@ -488,1 @@ gst_h265_parse_process_nal (GstH265Parse * h265parse, GstH265NalUnit * nalu) There are more callers of this function, where you probably want to check return values. But it's the same in h264parse, which you probably want to change at the same time then. @@ -973,3 @@ - nalu.type == GST_H265_NAL_PPS || - (h265parse->have_sps && h265parse->have_pps)) { - gst_h265_parse_process_nal (h265parse, &nalu); I think the intention here is that we skip all data except for the 3 headers in the beginning, and only after SPS/PPS are there we process everything else This was changed in h264parse for https://bugzilla.gnome.org/show_bug.cgi?id=732203 in 34c2cfd4ddcc370a273d5c86d403e421a94d0157 , so should be fine here for the same reasons
1. I am aware of it but h264parse do same( ignore return value in same situations) 2. So should I add something like this from h264parse: https://bug732203.bugzilla-attachments.gnome.org/attachment.cgi?id=279218 GST_H264_PARSE_STATE_VALID (h264parse,GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS)) ??
(In reply to m][sko from comment #3) > 1. I am aware of it but h264parse do same( ignore return value in same > situations) Yes, I'm aware of that. It seems wrong in both, that's what I meant above :) Maybe another patch to fix that for h264parse and do it right from the beginning in h265parse? > 2. > So should I add something like this from h264parse: > https://bug732203.bugzilla-attachments.gnome.org/attachment.cgi?id=279218 > > GST_H264_PARSE_STATE_VALID > (h264parse,GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS)) Keeping them more similar is certainly a good thing, yes.
Created attachment 333135 [details] [review] h265parse process more nalu types, parse state
I can't do more as I don't wont to break anything :)
Review of attachment 333135 [details] [review]: ::: gst/videoparsers/gsth265parse.c @@ -972,3 @@ - nalu.type == GST_H265_NAL_SPS || - nalu.type == GST_H265_NAL_PPS || - (h265parse->have_sps && h265parse->have_pps)) { This code makes it look like the VPS is optional, your changes assume it is mandatory. Which one is correct?
VPS NAL unit: https://github.com/GStreamer/gst-plugins-bad/blob/master/gst-libs/gst/codecparsers/gsth265parser.h#L469 I don't think VPS is uselless so old one looks bad to me.
Why is this a 'major' bug? Sounds more like a code refactoring/clean-up? Also, please could you provide a patch with proper name/attribution?
As we should be avare that h265parse is not in same state as h264parse :) Feel free and use this patche as yours
There is a difference with how the parameter sets work in H.264 and H.265. In H.265 the parameter sets are initially not active, they are only activated when they are referenced. An encoder is free to send a list of VPS, SPS and PPS nal units and only activate one parameter set. As a consequence, the encoder can send parameter updates at any time and any in any order. It is also not mandatory in HEVC main profile to activate the VPS, this parameter set can be ignored as all information is already present in the SPS. This patch breaks some of these concepts, the idea of refactoring both parsers in the same way might not be correct.
But purpose of h265parse should be just to parse data and analyze usefull information for gstreamer pipeline. h265encoder/h265decoder should handle everything else internaly.
There is a propose patch that makes a baseclass for the two parser, is this bug still relevant if we merge that ?
-- 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/416.