GNOME Bugzilla – Bug 708438
codecparsers: HEVC bitstream parser
Last modified: 2013-11-07 08:29:27 UTC
Created attachment 255367 [details] [review] HEVC bitstream parser The patch is adding HEVC(h265) bitstream parser to gst-plugins-bad. This is the initial implementation and a few things need to optimize (i guess). One eg: it is using dynamic allocation for slice_hdr and SEI message header. I would like to avoid it at least for slice_hdr. I have a minimal videoparser also based on this bitstream parser API which copied most of the code from h264parser. But didn't implement any format conversions yet.
Created attachment 256173 [details] [review] HEVC bitstream parser I have optimized a couple of things in the previous patch: -- Provide default_scaling_lists from pps if sps_scaling_list_enabled_flag=TRUE sps_scaling_list_data_present_flag=FALSE pps_scaling_list_data_present_falg=FALSE -- restricted the allowed value of a few fields by changing the types from guint32 to guint8 based on ProfileTierLevel constraints specified in Annex-A
Can you attach your h265parse element based on this too as a example of API usage?
The videoparser I wrote(copied from h264) is just to confirm the bitstream . I will try to make it as a proper element first and then create a new bug report for that. is that okay? Anyway, the usage would be similar to that of h264parser element except the case that we need to free the dynamically allocated memory for slice_hdr.
Created attachment 256789 [details] [review] Add HEVC(h265) videoparser element.
I have attached the hevc videoparser element implementation as another patch which copied most of the code from h264parser since both are similar in many cases. Still a few things needs to implement properly , for eg: SEI message handling and timestamp/duration adjustment based on SEI if the upstream didn't provide any valid timestamp/duration for the buffer. Remember i didn't do any extensive testing for this, but all the basic cases are working at least for me (samples are generated with the help of HM_12.0 and MP4Box). Anyway for now i put the rank as SECONDARY and later we can change it to PRIMARY once we fix all the existing issues (or at least the FIXMEs i have mentioned in the code). By the way i have a seen a couple of things which we need to optimize/correct in h264parser while copying the code to h265parser. One eg: the h264parser is doing all PAR calculation based on VUI header which is unnecessary since the bitstream parser is already doing that.
It would be good if some one can push this to upstream. It might not be stable enough(that will happen only step by strep, right?), but why can't we push it in to plugins-bad and that too with rank != primary. Otherwise i feared that this will also stay here for long time like what happened for mvc. We are not going to get any level of feed back on something which is not in the source code tree :(
Sure, we can get that in. Note that primary vs. secondary rank does not really matter at all, what matters is more none vs. a rank (will it be autoplugged and subjected to random incoming data or not) ;-) I noticed there's a bit of a naming inconsistency (also present in the H264 codec parser): GstH265NalParser * gst_h265_nal_parser_new (void); and then gst_h265_parser_identify_nalu(GstH265NalParser *nalparser,...) rather than gst_h265_nal_parser_identify_nalu (GstH265NalParser *nalparser,...) Any reason not to do GstH265NalParser -> GstH265Parser gst_h265_nal_parser_*() -> gst_h265_parser_*() ? (It's more similar to gst_h265_parse_*/GstH265Parse then, but I don't think that's a problem). If yes, no need for a patch, I can change it locally, was just wondering.
(In reply to comment #7) > Any reason not to do > > GstH265NalParser -> GstH265Parser > gst_h265_nal_parser_*() -> gst_h265_parser_*() > > ? > > (It's more similar to gst_h265_parse_*/GstH265Parse then, but I don't think > that's a problem). > > If yes, no need for a patch, I can change it locally, was just wondering. I can't see any specific reason for this. GstH265Parser/gst_h265_parse_* could be fine i guess. Only problem i can see is confusion which can introduce by the single letter difference between gst/videoparsers/gsth265parse.h/GstH265Parse structure and gst/codecparsers/gsth265parser.h/GstH265Parser structure. :) If you think the same, then it might be better to keep GstH265Nalparser as it is.
Either the other functions all get renamed to gst_h264_nal_parser_*() or the struct/object gets renamed to GstH264Parser, one of those. I'm not particularly concerned about the confusion with GstH265Parser, it's not public API.
s/264/265/
okay, Then better to use GstH265Parser/gst_h265_parser_* in my opinion. Will you change it locally?
Pushed with aforementioned modifications, left h265parse at secondary rank so it gets autoplugged. Thanks for the patches! commit d844832ec35847db9c8ad5b9b541e6a3e12616db Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Wed Oct 9 10:54:14 2013 +0300 videoparsers: add H.265 HEVC video parser element https://bugzilla.gnome.org/show_bug.cgi?id=708438 commit 33451e07912f5cc66c82b7278fa3ccd27e435e65 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Tue Oct 1 13:39:41 2013 +0300 codecparsers: add H.265 HEVC bitstream parser https://bugzilla.gnome.org/show_bug.cgi?id=708438
Cool ! Thanks for the push.. :)