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 736213 - h264parse: process NAL AU DELIMITER
h264parse: process NAL AU DELIMITER
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 696037 724510 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-09-06 23:33 UTC by m][sko
Modified: 2018-10-06 09:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
process NAL AU delimiter (824 bytes, patch)
2014-09-06 23:34 UTC, m][sko
needs-work Details | Review
inline au delimiter (5.10 KB, patch)
2014-09-12 23:00 UTC, m][sko
none Details | Review
inline au delimiter (4.97 KB, patch)
2014-09-12 23:35 UTC, m][sko
needs-work Details | Review
h264parse: insert AU delimiter only in case of byte-stream (3.26 KB, patch)
2017-02-17 03:11 UTC, Hyunjun Ko
none Details | Review
h264parse: insert AU delimiter only in case of byte-stream (2.78 KB, patch)
2017-02-21 01:21 UTC, Hyunjun Ko
none Details | Review
h264parse: Fix insertion of AU delimiters in alignment=au mode (2.24 KB, patch)
2017-03-02 13:56 UTC, Sebastian Dröge (slomo)
none Details | Review
h264parse: insert AU delimiter only in case of byte-stream (5.64 KB, patch)
2017-03-07 09:37 UTC, Hyunjun Ko
committed Details | Review
h264parse: fix some failures in testcases (2.14 KB, patch)
2017-03-08 08:20 UTC, Hyunjun Ko
committed Details | Review
tests: h264parse: fix failures due to insertion of au delimiter (7.93 KB, patch)
2017-03-08 08:21 UTC, Hyunjun Ko
committed Details | Review

Description m][sko 2014-09-06 23:33:59 UTC
there is no reason to dump AU delimiter until we get SPS PPS as 
AU is harmless NAL

for example:
AU SPS PPS IDR AU SLICE AU SLICE ..................
produce:
SPS PPS IDR AU SLICE AU SLICE ..................

iDevices don't play hls stream without AU delimiter for every frame
so If I have h264parse after encoder h264 parse dump first AU delimiter
Comment 1 m][sko 2014-09-06 23:34:31 UTC
Created attachment 285598 [details] [review]
process NAL AU delimiter
Comment 2 m][sko 2014-09-06 23:36:14 UTC
to 1.4 branch too plz
Comment 3 Olivier Crête 2014-09-09 14:54:27 UTC
Actually, we need to insert AU delimiters between AUs when outputting byte-stream mode to create a valid stream (for MPEG-TS embedding).
Comment 4 Sebastian Dröge (slomo) 2014-09-12 12:27:37 UTC
Yes
Comment 5 Sebastian Dröge (slomo) 2014-09-12 12:28:30 UTC
Comment on attachment 285598 [details] [review]
process NAL AU delimiter

Need to insert AU if required instead
Comment 6 m][sko 2014-09-12 23:00:14 UTC
Created attachment 286096 [details] [review]
inline au delimiter 

as we discuss in other bug 
https://bugzilla.gnome.org/show_bug.cgi?id=736211
I made this patch
Comment 7 m][sko 2014-09-12 23:35:02 UTC
Created attachment 286097 [details] [review]
inline au delimiter

as we discuss in other bug 
https://bugzilla.gnome.org/show_bug.cgi?id=736211
I made this patch
Comment 8 Olivier Crête 2014-09-15 21:49:37 UTC
Review of attachment 286097 [details] [review]:

There shouldn't be a property for this. The AUD should always be inserted in bytestream mode, and never in avc mode. And it should be inserted whatever the alignment is (au or nal), the question is just if it should be in the same GstBuffer or not.
Comment 9 m][sko 2014-09-16 07:01:34 UTC
1. ok so I will change
h264parse->align == GST_H264_PARSE_ALIGN_AU
=>
h264parse->format == GST_H264_PARSE_FORMAT_BYTE
in gst_h264_parse_pre_push_frame


2. gst_pad_push should be enaught.
 do you have problem with some muxers ?
 for example if you generate hls with mpegtsmux I think that this should be ok
that is reason why I flagged buffer SPS+PPS+IDR NALs with GST_BUFFER_FLAG_DELTA_UNIT
it is maybe useless btw
GST_BUFFER_FLAG_SET (buffer, GST_BUFFER_FLAG_DELTA_UNIT);
Comment 10 m][sko 2014-09-16 07:04:38 UTC
property or not
I saw some spec that AU delimiter is options
Comment 11 Sebastian Dröge (slomo) 2014-09-16 07:40:54 UTC
The AU delimiter should be in the *same* buffer for alignment=AU, otherwise things will break. Also this probably needs some changes to also insert the AU delimiter if in passthrough mode
Comment 12 Jan Schmidt 2015-07-23 16:31:40 UTC
*** Bug 724510 has been marked as a duplicate of this bug. ***
Comment 13 Jan Schmidt 2015-07-23 16:45:57 UTC
*** Bug 696037 has been marked as a duplicate of this bug. ***
Comment 14 Jan Schmidt 2015-07-23 16:48:35 UTC
Just a note, I just pushed:

commit 48a1f2792316d01c4ea4c6baa27188ffae73c543
Author: Jan Schmidt <jan@centricular.com>
Date:   Fri Jul 24 02:46:21 2015 +1000

    h264parse: Don't discard first AU delimiter
    
    Don't throw away AU delimiter(s) that precede the SPS/PPS. Should
    fix MPEG-TS playback on iOS/Quicktime when muxing streams that
    already have AU delimiters.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=736213 for getting
    h264parse to insert AU delimiters when they don't already
    exist.
Comment 15 Hyunjun Ko 2017-02-17 02:59:00 UTC
Hi. I'm from #Bug 776712 :)

I think if this issue can be fixed, it would be happy for all of us:)
If people don't mind, I would propose new patch based on master.
Comment 16 Hyunjun Ko 2017-02-17 03:11:08 UTC
Created attachment 346038 [details] [review]
h264parse: insert AU delimiter only in case of byte-stream

Inserts AU delimeter by default if missing au delimeter from upstream.
This should be done only in case of byte-stream format.
Comment 17 Víctor Manuel Jáquez Leal 2017-02-17 16:11:12 UTC
Review of attachment 346038 [details] [review]:

Also, since AU delimiter is optional, there should be a property, imo.

::: gst/videoparsers/gsth264parse.c
@@ +2329,3 @@
+  gboolean ok;
+
+  gst_byte_writer_init_with_size (&bw, gst_buffer_get_size (buffer), FALSE);

Isn't there an overflow? because you are adding 48 bits to the original buffers
Comment 18 Hyunjun Ko 2017-02-18 07:22:31 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #17)
> Review of attachment 346038 [details] [review] [review]:
> 
> Also, since AU delimiter is optional, there should be a property, imo.

Some people want that it should be by default. I have no idea of this thing. 
Hope that listen to opinion of someone else. :)

> 
> ::: gst/videoparsers/gsth264parse.c
> @@ +2329,3 @@
> +  gboolean ok;
> +
> +  gst_byte_writer_init_with_size (&bw, gst_buffer_get_size (buffer), FALSE);
> 
> Isn't there an overflow? because you are adding 48 bits to the original
> buffers

That size is not fixed as the third parameter, which means it can allocate more if needed. But I think you're right in this case, which can be fixed at initialization for performance.
Comment 19 Olivier Crête 2017-02-20 14:30:47 UTC
I think that AU delimiter should always be there in byte-stream mode as it's required to make a valid MPEG-TS file.

As for avc1/avc3 modes, I don't think it's required, I'd be tempted to never include it, but I have no strong feelings there.
Comment 20 Olivier Crête 2017-02-20 14:32:56 UTC
Review of attachment 346038 [details] [review]:

and you seem to be doing two copies. Maybe better just allocate a new GstMemory, write the AUD in there and append it to the buffer.
Comment 21 Tim-Philipp Müller 2017-02-21 00:38:37 UTC
> I think that AU delimiter should always be there in byte-stream
> mode as it's required to make a valid MPEG-TS file.

Where is this specified? (That it is required to make a valid MPEG-TS file)
Comment 22 Hyunjun Ko 2017-02-21 01:21:17 UTC
Created attachment 346292 [details] [review]
h264parse: insert AU delimiter only in case of byte-stream

Agree with Oliver about memory copy thing.
Patch updated
Comment 23 Nicolas Dufresne (ndufresne) 2017-02-21 12:16:06 UTC
(In reply to Tim-Philipp Müller from comment #21)
> > I think that AU delimiter should always be there in byte-stream
> > mode as it's required to make a valid MPEG-TS file.
> 
> Where is this specified? (That it is required to make a valid MPEG-TS file)

I didn't have time to dig into the spec, but at least Apple requires it for HLS:

https://developer.apple.com/library/content/documentation/NetworkingInternet/Conceptual/StreamingMediaGuide/FrequentlyAskedQuestions/FrequentlyAskedQuestions.html

I believe it's armless to have them. It's common to skip unused unit, but uncommon to add them when missing.
Comment 24 Nicolas Dufresne (ndufresne) 2017-02-21 12:16:38 UTC
(Forgot to mention, it's in question 10)
Comment 25 Tim-Philipp Müller 2017-02-21 14:24:58 UTC
I'm quite aware that Apple requires them (see various mailing list posts that led most people to this bug), and I also agree that it makes sense to add/output them by default for byte-stream output.

I just don't believe what Olivier said is true, that they are required to make a valid TS stream, but happy to be convinced otherwise :)
Comment 26 Olivier Crête 2017-02-27 20:26:39 UTC
This is specified in H.222 (06/2012) on in section 2.14.1 (page 135), the first bullet point is:

"Each AVC access unit shall contain an access unit delimiter NAL Unit;"
Comment 27 Víctor Manuel Jáquez Leal 2017-02-28 18:32:12 UTC
Given Olivier input the patch looks right. Can we land it?
Comment 28 Sebastian Dröge (slomo) 2017-03-01 21:14:10 UTC
Is that patch also ensuring the AU is there if alignment=nal instead of au?
Comment 29 Hyunjun Ko 2017-03-02 03:32:20 UTC
(In reply to Sebastian Dröge (slomo) from comment #28)
> Is that patch also ensuring the AU is there if alignment=nal instead of au?

Ah, you're right.
In case of alignment=nal, this patch inserts AUD to every nal unit, which is wrong.

I couldn't find a way to figure it out when it should insert AUD in case of alignment=nal because a frame means a nal unit in this case, which means
we should know if this nal unit is start of au or not and also if previous nal unit was AUD or not.

Any suggestion about this?
Or we can go this only in case of alignment=au?
Comment 30 Sebastian Dröge (slomo) 2017-03-02 05:55:15 UTC
(In reply to Hyunjun Ko from comment #29)

> Or we can go this only in case of alignment=au?

That would then still be invalid in the alignment=nal case


I think for that case it would have to be detected on the input side if the current AU started with an AU marker, and then add it before outputting the first NAL of the AU at the output side. Doing might need to add a little bit of buffering.
Comment 31 Sebastian Dröge (slomo) 2017-03-02 13:34:23 UTC
Comment on attachment 346292 [details] [review]
h264parse: insert AU delimiter only in case of byte-stream

Also this patch breaks some h264 streams I have here, looking
Comment 32 Sebastian Dröge (slomo) 2017-03-02 13:56:24 UTC
Created attachment 347065 [details] [review]
h264parse: Fix insertion of AU delimiters in alignment=au mode

We have to compensate for the new bytes added for the AU, otherwise
insertion of PPS/SPS will use wrong offsets and overwrite wrong data.

Also mark the AU delimiter blob const, and use frame->out_buffer for
storing the output to keep baseparse assumptions valid.

Still broken in alignment=nal mode.
Comment 33 Hyunjun Ko 2017-03-07 09:37:20 UTC
Created attachment 347370 [details] [review]
h264parse: insert AU delimiter only in case of byte-stream

Inserts AU delimeter by default if missing au delimeter from upstream.
This should be done only in case of byte-stream format.

Note that:
We have to compensate for the new bytes added for the AU, otherwise
insertion of PPS/SPS will use wrong offsets and overwrite wrong data.

Also mark the AU delimiter blob const, and use frame->out_buffer for 
storing the output to keep baseparse assumptions valid.

-----
I reprepared a patch for handling both alignment of nal and au.
I've unified slomo's patch since I changed the first proposed patch a bit.
Comment 34 Sebastian Dröge (slomo) 2017-03-07 11:22:41 UTC
Looks good to me. Now someone should check if this is also needed for h265 and open a bug for it in that case :) Hyunjun, Olivier, do you know?
Comment 35 Sebastian Dröge (slomo) 2017-03-07 11:27:47 UTC
Attachment 347370 [details] pushed as 201e71c - h264parse: insert AU delimiter only in case of byte-stream
Comment 36 Tim-Philipp Müller 2017-03-07 11:46:41 UTC
Thanks for the reference Olivier, I stand corrected :)
Comment 37 Sebastian Dröge (slomo) 2017-03-07 12:49:56 UTC
Unfortunately this breaks various tests:

$ make elements/h264parse.check
[...]
0%: Checks: 6, Failures: 6, Errors: 0
Comment 38 Hyunjun Ko 2017-03-08 05:29:55 UTC
(In reply to Sebastian Dröge (slomo) from comment #37)
> Unfortunately this breaks various tests:
> 
> $ make elements/h264parse.check
> [...]
> 0%: Checks: 6, Failures: 6, Errors: 0

Sorry for that.
I'm working on it.
Comment 39 Hyunjun Ko 2017-03-08 08:20:39 UTC
Created attachment 347445 [details] [review]
h264parse: fix some failures in testcases

Move logic of judgement whether or not to insert AU Delimiter before draining.
Besides, aud_needed flag is reset to TRUE when skip.
Comment 40 Hyunjun Ko 2017-03-08 08:21:26 UTC
Created attachment 347446 [details] [review]
tests: h264parse: fix failures due to insertion of au delimiter

Since insertion of aud landed, we need to change some testcases
accroding to the change.

Note that counting frames are changed in parser.c,
due to generated frames, AUD.
Comment 41 Sebastian Dröge (slomo) 2017-03-08 12:20:41 UTC
Attachment 347445 [details] pushed as a997a99 - h264parse: fix some failures in testcases
Attachment 347446 [details] pushed as 08219f3 - tests: h264parse: fix failures due to insertion of au delimiter
Comment 42 Nicolas Dufresne (ndufresne) 2018-09-21 01:31:55 UTC
I've been testing this today with multiple slices. With 2 slice, this code insert an AU delimiter right in between the two slices for me, and the second slice end up with a bumped forward timestamp.
Comment 43 Tim-Philipp Müller 2018-10-05 12:01:22 UTC
Maybe we can clone this into a new bug instead of re-opening one from 18 months ago? :)
Comment 44 Nicolas Dufresne (ndufresne) 2018-10-05 21:00:04 UTC
Sure, in fact let's close, I'll make one ticket with my h264/h265parse enhancement (hopefully next week). I also implemented the same for h265.