I didn't look closely yet but you're reusing a lot of code from the mpeg ts demuxer. Would it be possible to put the plugin into the mpeg ts demuxer plugin and share the code instead of copying it?

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

That would be nice in general, but there are a few problems:
1. Small changes here and there will have to be merged and accepted
by the maintainer. Since I am a newcomer here, I have no idea how
difficult is that.
2. I am reusing the code from two plugins: mpegtsmux(fluendo) and
mpegtsdemux(Edward Hervey), so it is not possible to move them
into a single place and avoid duplication.

Is there any place for the common code shared between several plugins?

I guess there is an option to move the depayloader and
parser to demux, and payloader - to muxer, as that's the
way the code was reused.
I'll take a look into that possibility.

The best would probably be to have all code in the new mpegts demuxer plugin, I think most of the PES related changes will apply to the ts demuxer too.

That will mean still the duplication of the mpegtsmux code though.
But I'll go that way because I wanted to keep all the PES plugins
in a single place.
Will post the patch soon.

Oh sorry, ignore what I said. Putting the parser and depayloader into the tsdemux plugin and the payloader into the tsmux plugin absolutely makes sense

Created attachment 194815 [details] [review]
do not use same arg for input and output

Created attachment 194816 [details] [review]
patch [2/4]

Created attachment 194817 [details] [review]
tsmux: delimit max size of pes packets

Created attachment 194818 [details] [review]
patch [4/4]

Done, please take a look.

Created attachment 194875 [details] [review]
patch [2/4]

depayloader v2

Created attachment 194887 [details] [review]
patch [2/4]

Created attachment 195325 [details] [review]
fix pes startcodes in gstmpegdefs.h

The pesparse.c has this code line:
 res->stream_id = val32 & 0x000000ff;
but gstmpegdefs.h defines stream IDs and start codes
with the 16th bit set. This probably came from the
fluendo demuxer code, which reads start code as a full
(not masked) 32bit value.
Currently, these defines are not used in mpegtsdemux code,
so the fix is needed only for the further development of
the PES parser.

Created attachment 196643 [details] [review]
fix parsing pes extension field

It seems there is some confusion in the pesparser code of
tsdemux plugin... I don't know what this part was supposed
to do at all:
---
      flags = *data++;
      /* Only valid if stream_id_extension_flag == 0x0 */
      if (!(flags & 0x80)) {
        res->stream_id_extension = flags & 0x7f;
---
If bit 7 is not set (!(flags & 0x80)), there is no use of
doing flags & 0x7f on the next line, is it?
Moreover, I failed to track that to the description of the
pes header format, and in fact mpegtsmux can write different
values there: 0x60, 0x71, 0x80 and 0x82 to specify the media
type, so the 0x7f mask cannot be applied.
So the code looks totally wrong and misplaced, probably a
copy/paste bug.
the attached patch removes the code in question, allowing my
plugin to get the media type.

I am removing the plugins implementation for now: will re-upload
when they are finished. Let's see if at least the fixes can be
processed and applied to that date.

Review of attachment 196643 [details] [review]:

::: gst/mpegtsdemux/pesparse.c
@@ +337,2 @@
     if (res->extension_field_length) {
+      res->stream_id_extension_data = data;

There seems to be something missing here.

The original code does:
* read and advance data by 1 byte
* advance data by res->extension_field_length

Your code does:
* read data byte
* advance data by res->extension_field_length

Review of attachment 195325 [details] [review]:

Seems acceptable. I'm even wondering if we shouldn't just convert those values to 8 bit values if we're only going to be using the lower eight bits.

Based on comment 18, should I disregard comment 17?
If not - please clarify what I need to do. :)
You marked the patch as needs-work, so the question.
(yes, the original code was reading an extra byte)

Created attachment 198356 [details] [review]
tsmux: use sequential stream ids for pes

There is a /* FIXME: Assign sequential IDs? */ in code.
This patch does exactly that.

Created attachment 198725 [details] [review]
[mpegtsmux] allow re-using prepare funcs from other plugins

This patch allows to re-use mpegtsmux_prepare_aac() and
mpegtsmux_prepare_h264() from other plugins, like mpegpespay.

(In reply to comment #17)
> Review of attachment 196643 [details] [review]:
Sorry, I haven't answered this one because of some
confusion with another comment...

> ::: gst/mpegtsdemux/pesparse.c
> @@ +337,2 @@
>      if (res->extension_field_length) {
> +      res->stream_id_extension_data = data;
> 
> There seems to be something missing here.
> 
> The original code does:
> * read and advance data by 1 byte
> * advance data by res->extension_field_length
Yes.

> Your code does:
> * read data byte
The extension_data[0] byte.

> * advance data by res->extension_field_length
Yes, because the res->extension_field_length==1 usually.
And if it is >1, then this code is still correct: all it
does is to bypass the extension_data bytes. The byte we
read above, is just the extension_data[0] byte, so it doesn't
require an extra increment. We read first byte of an array
and then skip the entire array, so that is fine.

But I can see the line:
---
     length -= res->extension_field_length + 1;
---
which would need to remove +1 then...
I'll upload the patch with this fixed.

Created attachment 232654 [details] [review]
fix parsing pes extension field

Hopefully a better version of the patch.

(In reply to comment #22)
> > Your code does:
> > * read data byte
> The extension_data[0] byte.
Grr, wrong! I've forgot that code entirely.
It doesn't read the byte:
---
res->stream_id_extension_data = data;
---
it just puts a pointer to extenstion_data into
res->stream_id_extension_data.
So now I added a wrong comment into a patch, will re-upload again...

Created attachment 232657 [details] [review]
fix parsing pes extension field

With the new 1.0, I think we should handle PES in a different way (i.e. not a new caps type).

We should have a well-know GstMeta for PES (GstPESMeta ?) which:
* includes parsed header structure
* points to actual PES header data in the underlying GstBuffer GstMemory

The memory of the buffers created by PES-aware elements should:
* Contain all the data (including PES header)
* have the offset point to the actual data (i.e. memory offset == size(PES header).

This would:
* allow non-pes aware elements to still work as expected (no special path)
* pes-aware elements to make usage of the PES information

Then we would still need a "pesparse" element that can create such buffers, otherwise sounds like a good idea :)

But the small fixes that are sitting here
for 2 years already, have nothing to do with
the way you are going to handle PESes.
So why not to apply them?

If they have nothing to do with adding PES support, they should probably go into another bug :-)

That's what was done: the PES-supporting
patch went to 661163 from here, and I left only the
"unrelated" fixes here.

(In reply to comment #29)
> If they have nothing to do with adding PES support, they should probably go
> into another bug :-)
Done: entry renamed.

Comment on attachment 195325 [details] [review]
fix pes startcodes in gstmpegdefs.h

we no longer have those ids.

Comment on attachment 194815 [details] [review]
do not use same arg for input and output

Thanks for pointing this out. I ended up pushing a variant which just completely removes the offset argument (we always used it with offset == 0)

commit 5208b8a05074ebdb0da835c29b95f8eb6001cb0e
Author: Edward Hervey <edward@collabora.com>
Date:   Wed Aug 14 10:33:14 2013 +0200

    pesparse: Remove unused argument
    
    We always provided 0 as the offset and never used the returned value.
    
    Based on feedback from Stas Sergeev <stsp@list.ru>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=657343

Comment on attachment 232657 [details] [review]
fix parsing pes extension field

We actually want to keep the extension data around.

I did take into account the length check fix in this commit:

commit ddee83ef0bc13ce888a890f29650454e2c7ec496
Author: Edward Hervey <edward@collabora.com>
Date:   Wed Aug 14 10:39:46 2013 +0200

    pesparse: Fix pes extension data length check
    
    And remove length/data updates (we use the header size just below to
    properly set them).
    
    Based on feedback from Stas Sergeev <stsp@list.ru>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=657343

Only mpegtsmux patches remain.

(In reply to comment #34)
> (From update of attachment 232657 [details] [review])
> We actually want to keep the extension data around.
I don't understand.
My patch also kept the data around, but it also fixed
the bug.
Could you please answer to Comment 16?

../../gst-libs/gst/mpegts/mpegts.h:32:44: fatal error: gst/mpegts/gstmpegts-enumtypes.h: No such file or directory

Where is this file? The code doesn't compile for me.

Run 'make' from the top-level directory. the -enumtypes.[ch] files are generated at build time using glib-mkenum.

(In reply to comment #16)
> It seems there is some confusion in the pesparser code of
> tsdemux plugin... I don't know what this part was supposed
> to do at all:
> ---
>       flags = *data++;
>       /* Only valid if stream_id_extension_flag == 0x0 */
>       if (!(flags & 0x80)) {
>         res->stream_id_extension = flags & 0x7f;
> ---
> If bit 7 is not set (!(flags & 0x80)), there is no use of
> doing flags & 0x7f on the next line, is it?

  Correct, fixed that locally.

  The usage of PES_extension_field_length was also still a bit weird. The docs are a bit conflicting (the field description says it's the number of bytes after *that* field, but the PES packet table (2-21) is stating something else).

> Moreover, I failed to track that to the description of the
> pes header format, and in fact mpegtsmux can write different
> values there: 0x60, 0x71, 0x80 and 0x82 to specify the media
> type, so the 0x7f mask cannot be applied.

  mpegtsmux is doing something wrong if it's setting a stream_id_extension with the highest bit set. That needs fixing. tsdemux/pesparse is doing the expected thing.

Please see tsmux_stream_new().
It writes the correct values to stream->id_extended, some
of these values have bit 7 set, and that looks fine.
What is not fine, is that your parser expects some kind
of "flags" at that point, but there is simply no any flags.
Or if there are - what spec says that?
My patch simply removes the parsing of a non-existent flags.
Otherwise you don't get stream->id_extended from tsmux.

http://www.itu.int/rec/T-REC-H.222.0-201206-I/en  (Latest public specifications June 2012)

Not cool really. I was expecting you were relying on specifications instead of guessing what's going on based on misc code.

If you want to discuss such details => IRC or mailing list.

In the meantime I just pushed a cleanup for tsdemux.

commit 21ebc7708d27fc42dcbcc4f802052c46c0229735
Author: Edward Hervey <edward@collabora.com>
Date:   Wed Aug 14 13:41:37 2013 +0200

    pesparse: Refactory secondary PES extension handling
    
    Some streams had wrong values for the stream_id_extension, make sure
    we only remember the valid ones.
    
    For streams with PES_extension_field_length == 0, assume there's nothing
    else.
    
    For streams that state they have a TREF extension but don't have enough
    data to store it, just assume it was produced by a non-compliant muxer
    and skip the remaining data.
    
    Only store remaining data in stream_id_extension_data instead of storing
    data we already parse.

(In reply to comment #41)
> Not cool really. I was expecting you were relying on specifications instead of
> guessing what's going on based on misc code.
Do you really expect me to remember the specification 2
years after I made the patch?

Anyway, the iso13818-1 says this:
---
if ( PES_extension_flag_2 == '1') {
  marker_bit
  PES_extension_field_length
  for (i = 0; i < PES_extension_field_length; i++) {
    reserved
  }
}
---

But the document you point to, says this:
---
if ( PES_extension_flag_2 == '1') {
  marker_bit
  PES_extension_field_length
  stream_id_extension_flag
  If ( stream_id_extension_flag == '0') {
    stream_id_extension
  }
  else {
---

It is up to you to decide what document is right, but
accusing me in guessing on a misc code is not correct.
This "misc code" is from your git, and the standard is
from ISO.

Note that your document is messed up in that point.
It has this a few lines below:
---
    for (i = 0; i < PES_extension_field_length; i++) {
      reserved
    }
}
      }
---
Here, the closing brackets are messed (page 35), so I
wouldn't trust that part at all.

Edward, what should be done here?

-- 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/44.