Created attachment 285598 [details] [review]
process NAL AU delimiter

to 1.4 branch too plz

Actually, we need to insert AU delimiters between AUs when outputting byte-stream mode to create a valid stream (for MPEG-TS embedding).

Yes

Comment on attachment 285598 [details] [review]
process NAL AU delimiter

Need to insert AU if required instead

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

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

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.

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);

property or not
I saw some spec that AU delimiter is options

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

*** Bug 724510 has been marked as a duplicate of this bug. ***

*** Bug 696037 has been marked as a duplicate of this bug. ***

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.

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.

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.

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

(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.

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.

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.

> 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)

Created attachment 346292 [details] [review]
h264parse: insert AU delimiter only in case of byte-stream

Agree with Oliver about memory copy thing.
Patch updated

(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.

(Forgot to mention, it's in question 10)

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 :)

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;"

Given Olivier input the patch looks right. Can we land it?

Is that patch also ensuring the AU is there if alignment=nal instead of au?

(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?

(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 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

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.

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.

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?

Attachment 347370 [details] pushed as 201e71c - h264parse: insert AU delimiter only in case of byte-stream

Thanks for the reference Olivier, I stand corrected :)

Unfortunately this breaks various tests:

$ make elements/h264parse.check
[...]
0%: Checks: 6, Failures: 6, Errors: 0

(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.

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.

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.

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

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.

Maybe we can clone this into a new bug instead of re-opening one from 18 months ago? :)

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.