Created attachment 279127 [details] [review]
h264parse: default to byte-stream/nalu format (Annex B)

alignment=nal does not mean that each buffer contains a single NAL unit.

(In reply to comment #2)
> alignment=nal does not mean that each buffer contains a single NAL unit.

Interesting, it seems to be the case right now, isn't it? i.e. we get nonext = TRUE in that case after having parsed the current NAL, and break before getting to the next one and submit the frame.

I don't think it's always the case in practice, and it's just not what it means either. I have a vague memory of someone once adding an optimisation based on that assumption and it turned out to be false and break things.

Btw, I wasn't commenting on the code or the patch. It may well be true in the context of the code you're dealing with, I was making a general statement about buffers between elements with those caps.

OK, thanks for the clarification. Though, I still believe that having a granularity by which GstBuffer = NALU would be much simpler to deal with. In my experience, and reading through the code, GstBuffer = AU is not behaving correctly. That's also what I am working on BTW. :)

Created attachment 279174 [details] [review]
h264parse: default to byte-stream/nalu format (Annex B)

Updated patch to fix gst_h264_parse_negotiate() default to alignment=AU for avcC format, and alignment=NAL for byte-stream. Also fixed the comment to not claim anything on the resulting GstBuffer representation.

I always though alignment=nal meant one nal per buffer. Imho, it would make a lot more sense, otherwise it's not that useful.

Ack, alignment=nal should be one NALU per buffer. Otherwise we could as well call it alignment=random :)

How does this block bug #731831?

Review of attachment 279174 [details] [review]:

Can be pushed after 1.4.0 release, mostly because it could break some code that assumes the current behaviour and because ↓

Also we should decide on how to handle alignment=nal in general, and make sure h264parse is doing the right thing here.

::: gst/videoparsers/gsth264parse.c
@@ +369,3 @@
   if (!align)
+    align = format == GST_H264_PARSE_FORMAT_BYTE ? GST_H264_PARSE_ALIGN_NAL :
+        GST_H264_PARSE_ALIGN_AU;

Add some brackets here, even if not requirede :)

(format == BYTE) ? NAL : AU

(In reply to comment #10)
> How does this block bug #731831?

Well, the original testing tool from QA was testing the default choices from filesrc ! h264parse ! vaapidecode ! vaapisink. Choosing a default and natural configuration for that was appealing and could have improved the PASS rate. Anyway, I have now made the requirement to test all 3 combinations explicitly.

In summary: that was not really blocking either, just wanted the people to track and collect all the required patches for testing. :)

Ok, but how would a different default choice make the tests fail or succeed? That's because of the other bugs for which you provided patches, right?

(In reply to comment #13)
> Ok, but how would a different default choice make the tests fail or succeed?
> That's because of the other bugs for which you provided patches, right?

Because the new defaults could work without additional patches. i.e. all raw NAL units were correctly communicated as is to the decoder. In other cases (alignment=au), we dropped SPS when a buffering_period() was received before PPS, among another related issue that was addressed in that mode too.

Right, so it would work around the other bugs :)

Why is this patch still hanging in bugzilla??? Needs the same for HEVC too.

What should happen with this? Tim, Olivier?

I'd say ++ on this patch. And clearly document that alignment=nal mean 1-and-only-1 NAL per GstBuffer.

Comment on attachment 279174 [details] [review]
h264parse: default to byte-stream/nalu format (Annex B)

Does not apply anymore. Please update :) And the same patch for h265parse please

Created attachment 317054 [details] [review]
videoparsers: h264: default to byte-stream/nalu format (Annex B).

Rebased on current master..

Thing is, as far as I know e.g. tsdemux may output buffers that contain multiple NALs in one single buffer, because that's how they were packed. First random file I can find:

$ gst-launch-1.0 filesrc location=zdf-hd-sweden-test2.ts ! tsdemux ! video/x-h264 ! fakesink dump=true num-buffers=1 -q | head -n 5
00000000: 00 00 00 01 09 50 00 00 00 01 06 00 01 c0 01 01  .....P..........
00000010: 04 04 0a b5 00 31 44 54 47 31 41 f8 ff 04 0e b5  .....1DTG1A.....
00000020: 00 31 47 41 39 34 06 cf c0 00 c0 00 ff 80 00 00  .1GA94..........
00000030: 00 01 01 a8 03 84 97 92 7f fb 6f c1 f0 cb 19 3e  ..........o....>
00000040: 0b 5d 31 e0 66 3f f5 aa fc 8a 19 2a cb 5b ca 6a  .]1.f?.....*.[.j

So as far as I can tell we can't just redefine alignment=nal to mean "1 nal per buffer" however much we might want things to work that way, unless we want to add parsing to tsdemux. It's also called "alignment" for a reason, which implies just that - alignment, not packetisation.

Created attachment 317060 [details] [review]
videoparsers: h265: default to byte-stream/nalu format (Annex B)

There are other things which requires sync with h264parse, but anyway we have another bug to track it https://bugzilla.gnome.org/show_bug.cgi?id=754124..

(In reply to Sebastian Dröge (slomo) from comment #9)
> Ack, alignment=nal should be one NALU per buffer. Otherwise we could as well
> call it alignment=random :)

Just a side note, when we say "align on 4 bytes" we don't mean to allocate 4 bytes per buffer, but that the start of a buffer is a multiple of 4 (and sometimes it also means the size of the buffer is a multiple of 4, just so the next buffer can be aligned without padding). The use of the term alignment here could have easily mean "nal" for one of more NALU (with start code), "au" for one or more frame with start code. Though, considering how our decoder work, there is a need to define the packetization. It seems we did that happen by making alignment AU and avc being a single frame (which is not longer just an alignement). It's a bit counter intuitive.

So with the enclose patch, will h264parse default to create one GstBuffer per nal ? I would believe this would reduce throughput of simple pipeline like h264parse ! filesink no ? Also, it leaves tsdemux with no model to implement the optimal path for tsdemux ! filesink.

The current h264parse code is broken for MVC use cases since we keeping avc as the default format. Because we don't have the correct implementation to transform byte-stream to packetized format for MVC streams.

For MVC streams, h264parser is trying to convert byte-stream to avc, but the code is only considering the AVCDecoderConfigurationRecord format. For MVC streams, the packing should be based on MVCDecoderConfigurationRecord.

Unfortunately, the spec for the AVC/MVC{DecoderConfigurationRecord} is still not publically available. I have an old version (2010 amendment) for referring the MVCDecoderConfigurationREcord which is already surpassed by the 2016 version. So I am not sure what is the final form of MVCDecoderConfigurationRecord.

Note: We won't hit any issues until (a)there is a stream come up with a long list of SPS/SUBSET headers, (b) the downstream element try to use the MVCDecdoerConfigurationRecord as it is to interpret the codec_data.

Our codec-data generation is based on AVCDecoderConfigurationRecord,
As per that codec_data[5]: 
   bit(3) reserved = ‘111’b;
    unsigned int(5) numOfSequenceParameterSets;

Based on the old version of spec which I have, codec_data[5] for MVC streams:
bit(1) reserved = ‘0’b;
unsigned int(7) numOfSequenceParameterSets;

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/155.