GNOME Bugzilla – Bug 721715
h264parse: Multiple SEI messages in SEI RBSP
Last modified: 2014-03-21 15:48:40 UTC
Created attachment 265549 [details] [review] h264parse: Fix SEI messages parsing Hi, An SEI RBSP could contains one or more SEI messages in SEI RBSP as specified in 7.4.2.3.1 However the h264 parser only parse the first message and ignore the others. The hack i attached parse more than one message and modify the h264 codecparser API. The gst_h264_parser_parse_sei() function now fill a GList instead of filling a single H264SEIMessage. I think this patch needs works since it modify the API (maybe a GList is not a good choice...) and some bugs remains. Indeed i have some troubles to determine the end of the SEI RBSP. It apply on 1.2.2 and master. Regards
Can you provide a sample file that has multiple SEI? Instead of a GList, maybe we can make this an array instead? Or a GPtrArray?
Created attachment 265681 [details] MPEG-TS containing h264 with multiple SEI messages Here is a MPEG-TS file which contains an H264 track with multiple SEI messages in one SEI RBSP. I use following pipeline to analyse SEI parsing: gst-launch-1.0 --gst-debug h264parse:6,codecparsers_h264:6 filesrc location=h264_with_multiple_sei.ts ! tsdemux ! h264parse ! fakesink
Created attachment 265701 [details] [review] h264parse: Fix multiple SEI messages in one SEI RBSP. I agree for the use of GArray instead of GList, it is more simple. Patch has been updated. Also i have a parsing bug which could be caused by more_rbsp_data() implementation as described in bug 685890: https://bugzilla.gnome.org/show_bug.cgi?id=685890 For example a SEI NAL payload (extracted from ts file i provided) is: "00 01 C0 01 01 14 06 01 C4 80 00" It contains 3 messages: buffering period (00 01 C0), picture timing (01 01 14) and recovery point (06 01 C4). After the third message, I think more_rbsp_data() should return FALSE, but actually it returns TRUE because there is more than 8 bits remaining. I will investigate this issue.
I apply the patch provided in bug 685890. It fixes my problem. However I don't know if it's the right implementation of more_rbsp_data(). Furthermore, i checked the h264 reference decoder provided by the ITU, and I notice they rely on an rbsp_trailing_byte syntax (0x80) instead of more_rbsp_data() to stop SEI RBSP parsing. Should we do the same, or should we work on more_rbsp_data() ?
Ok, let's discuss that in bug #685890 , get that one fixed first and then this one. I don't know the answer to your question though :)
Commit related to bug #721384 fixed my parsing issue.
Ok, what's left to be done here then? Is bug #685890 still related, or do you just need to update the patch here or is everything ready to be reviewed and pushed? :)
Created attachment 267041 [details] [review] h264parse: Fix multiple SEI messages in one SEI RBSP parsing. Finally, bug #685890 don't seems to immpact this patch since trailling 0x00 are striped. Also, I updated the patch to handle and return errors. So i think, you could review it. :)
Comment on attachment 267041 [details] [review] h264parse: Fix multiple SEI messages in one SEI RBSP parsing. I think you're missing something in the patch :) gsth264parser.c: In function 'gst_h264_parser_parse_sei_message': gsth264parser.c:1202:7: error: implicit declaration of function 'nal_reader_skip_to_byte' [-Werror=implicit-function-declaration] nal_reader_skip_to_byte (nr); ^ gsth264parser.c:1202:7: error: nested extern declaration of 'nal_reader_skip_to_byte' [-Werror=nested-externs] Also skip_to_byte() sounds like it would do nothing if the current position is byte aligned. You use it as a way to skip forward over bytes though. Maybe skip_to_next_byte()?
(In reply to comment #9) > (From update of attachment 267041 [details] [review]) > I think you're missing something in the patch :) > > Also skip_to_byte() sounds like it would do nothing if the current position is > byte aligned. You use it as a way to skip forward over bytes though. Maybe > skip_to_next_byte()? My mistake, I didn't notice that nal_reader_skip_to_byte() has been removed in master. For information, the patch was written and mainly tested on the 1.2 branch. It seems that I have forgotten to update my master workspace when i tested it on master :(. I will modify the patch and add a skip_to_next_byte() function. I'm sorry for this stupid mistake.
Created attachment 267061 [details] [review] h264parse: Fix multiple SEI messages in one SEI RBSP parsing. Updated to work on master. I add the nal_reader_skip_to_next_byte() function. Ii is the same code as nal_reader_skip_to_byte() in the 1.2 branch. So now this patch will apply on master but it will cause a conflict on 1.2 about the above function name.
Review of attachment 267061 [details] [review]: Well, the other patch is good for 1.2 :) Generally looks good but: ::: gst-libs/gst/codecparsers/gsth264parser.c @@ +280,3 @@ + } + + nr->bits_in_cache = 0; Isn't this skipping only to the next byte if byte-aligned... and otherwise skip to the *previous* byte?
(In reply to comment #12) > Review of attachment 267061 [details] [review]: > > Well, the other patch is good for 1.2 :) > > Generally looks good but: > > ::: gst-libs/gst/codecparsers/gsth264parser.c > @@ +280,3 @@ > + } > + > + nr->bits_in_cache = 0; > > Isn't this skipping only to the next byte if byte-aligned... and otherwise skip > to the *previous* byte? I think no because nal_reader_read() increments its 'byte cursor' after reading a byte and it 'copies' the byte read to the cache. So set the bits_in_cache to 0 will force nal_reader_read() to read the next byte. I don't know if i'm clear, if not, don't hesitate to ask.
Oh right, sorry :) Will backport to 1.2 later commit af78b45979564035ce4f2535d7257201112f776b Author: Aurélien Zanelli <aurelien.zanelli@parrot.com> Date: Fri Jan 3 09:44:28 2014 +0100 h264parse: Fix multiple SEI messages in one SEI RBSP parsing. An SEI RBSP could contains more than one SEI message as specified in 7.4.2.3.1. This commit change the parser API: the gst_h264_parser_parse_sei() function now create and fill a GArray containing GstH264SEIMessage. https://bugzilla.gnome.org/show_bug.cgi?id=721715
> Oh right, sorry :) Will backport to 1.2 later I don't think we should be backporting API changes into the stable branch, even if it's API marked as 'unstable'
Good point
Or you could just offer the new API under a different name and mark the old one as deprecated. Then you can add the new one to the stable branch as well.
> > > Oh right, sorry :) Will backport to 1.2 later > > > > I don't think we should be backporting API changes into the stable branch, even > > if it's API marked as 'unstable' > > Good point. Yet it still ended up in the stable release somehow it seems ...
nal_reader_skip_to_next_byte() is wrong, the payloadSize does not take into account the emulation prevention bytes. Why not just use nal_reader_skip(nr, payload_size)? And, this indeed fixes several regressions.