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 721715 - h264parse: Multiple SEI messages in SEI RBSP
h264parse: Multiple SEI messages in SEI RBSP
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.2.2
Other Linux
: Normal normal
: 1.2.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-07 17:22 UTC by Aurélien Zanelli
Modified: 2014-03-21 15:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
h264parse: Fix SEI messages parsing (10.13 KB, patch)
2014-01-07 17:22 UTC, Aurélien Zanelli
needs-work Details | Review
MPEG-TS containing h264 with multiple SEI messages (1.56 MB, video/mp2t)
2014-01-08 10:17 UTC, Aurélien Zanelli
  Details
h264parse: Fix multiple SEI messages in one SEI RBSP. (10.07 KB, patch)
2014-01-08 13:06 UTC, Aurélien Zanelli
none Details | Review
h264parse: Fix multiple SEI messages in one SEI RBSP parsing. (10.26 KB, patch)
2014-01-23 12:57 UTC, Aurélien Zanelli
needs-work Details | Review
h264parse: Fix multiple SEI messages in one SEI RBSP parsing. (10.72 KB, patch)
2014-01-23 16:35 UTC, Aurélien Zanelli
committed Details | Review

Description Aurélien Zanelli 2014-01-07 17:22:03 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
Comment 1 Sebastian Dröge (slomo) 2014-01-08 09:22:15 UTC
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?
Comment 2 Aurélien Zanelli 2014-01-08 10:17:12 UTC
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
Comment 3 Aurélien Zanelli 2014-01-08 13:06:15 UTC
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.
Comment 4 Aurélien Zanelli 2014-01-13 14:27:57 UTC
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() ?
Comment 5 Sebastian Dröge (slomo) 2014-01-14 10:02:09 UTC
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 :)
Comment 6 Aurélien Zanelli 2014-01-23 08:48:30 UTC
Commit related to bug #721384 fixed my parsing issue.
Comment 7 Sebastian Dröge (slomo) 2014-01-23 09:14:27 UTC
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? :)
Comment 8 Aurélien Zanelli 2014-01-23 12:57:25 UTC
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 9 Sebastian Dröge (slomo) 2014-01-23 13:24:42 UTC
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()?
Comment 10 Aurélien Zanelli 2014-01-23 13:55:57 UTC
(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.
Comment 11 Aurélien Zanelli 2014-01-23 16:35:18 UTC
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.
Comment 12 Sebastian Dröge (slomo) 2014-01-23 17:15:52 UTC
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?
Comment 13 Aurélien Zanelli 2014-01-23 18:06:42 UTC
(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.
Comment 14 Sebastian Dröge (slomo) 2014-01-23 19:19:21 UTC
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
Comment 15 Tim-Philipp Müller 2014-01-23 19:36:41 UTC
> 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'
Comment 16 Sebastian Dröge (slomo) 2014-01-24 08:10:52 UTC
Good point
Comment 17 Tim-Philipp Müller 2014-01-24 11:05:55 UTC
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.
Comment 18 Tim-Philipp Müller 2014-02-11 23:54:42 UTC
> > > 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 ...
Comment 19 Gwenole Beauchesne 2014-03-21 15:48:40 UTC
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.