With the new helper functions from pbutils, this should be rather simple now

This looks very interesting.

Luis: are you still working on this?

Yes I am :)

Created attachment 315683 [details] [review]
work in progress patch

Just a status update since Sebastian and Reynaldo are interested in this bug.

For clarity, the above patch is unfinished and just sharing the current progress state.

Still pending is populating the OpusSampleEntry box.

Created attachment 315759 [details] [review]
isomp4: add support for Opus in mp4mux

To test this patch:
gst-launch-1.0 videotestsrc num-buffers=500 ! video/x-raw,width=320,height=240 ! x264enc ! mp4mux name=mux ! filesink location=test.mp4 audiotestsrc wave=5 num-buffers=1000 ! audio/x-raw,channels=2,rate=48000 ! audioconvert ! opusenc ! mux.

And then play the generated test.mp4 file with git vlc.

I will add playback support for Opus in qtdemux in the next few days.

Review of attachment 315759 [details] [review]:

::: gst/isomp4/gstqtmux.c
@@ +3685,3 @@
+
+    ext_atom = build_opus_extension (rate, channels, mapping_family,
+        stream_count, coupled_count, channel_mapping, 0, 0);

These two zeroes here are not good :) The first zero would ideally be calculated from the GstAudioClippingMeta of the first buffers, the second we might want to add to the caps. Alternatively you can require a OpusHead streamheader in the caps (like oggmux/matroskamux) and read the two values from there. Transmuxing from e.g. RTP would require using opusparse then.

::: gst/isomp4/gstqtmuxmap.c
@@ +136,3 @@
+  "audio/x-opus, " \
+  "channels = (int) [1, 8], " \
+  "channel-mapping-family = (int) {0, 1};"

In theory we should also support other families and more channels

Created attachment 315775 [details] [review]
now we read the pre_skip and gain from the OpusHeader

Thanks for the review Sebastian!

Fixed the two issues you mentioned.

Review of attachment 315775 [details] [review]:

Main question remaining now is if anything special with the timestamps is needed like with Matroska, because of the pre-skip and padding for the last frame. Our timestamps will start at 0 and duration of the first frame will be shorter, and then the last frame will also have a shorter duration. For Matroska for example, the container wants timestamps/durations that include the pre-skip and padding.

::: gst/isomp4/gstqtmux.c
@@ +3686,3 @@
+        !GST_VALUE_HOLDS_ARRAY (streamheader) ||
+        gst_value_array_get_size (streamheader) == 0) {
+      GST_ERROR ("no streamheader field in caps %" GST_PTR_FORMAT, caps);

GST_ERROR_OBJECT()

@@ +3687,3 @@
+        gst_value_array_get_size (streamheader) == 0) {
+      GST_ERROR ("no streamheader field in caps %" GST_PTR_FORMAT, caps);
+      goto refuse_caps;

We could also just make this a warning and assume that the two values are 0 and just use the caps instead

Created attachment 315847 [details] [review]
[WIP] isomp4: add support for Opus in mp4mux

Latest version. Applied the fix to parsing the caps as a fallback when there isn't a streamheader.

When generating files with audiotestsrc this works. When creating them by transcoding a file, vlc complains that the Opus stream is corrupted. Surely to do with what Sebastian points out about timestamps.

Will continue debugging this tomorrow.

Review of attachment 315847 [details] [review]:

Needs work.

Review of attachment 315847 [details] [review]:

Merged:
commit 5ed8cba024c001e8d40f4189f0e5168893e772aa
Author: Luis de Bethencourt <luisbg@osg.samsung.com>
Date:   Mon Nov 16 13:26:50 2015 +0000

    isomp4: add support for Opus in mp4mpux

    Add support for muxing MP4 files containing Opus. Based on the spec
    detailed here:
    https://www.opus-codec.org/docs/opus_in_isobmff.html

    https://bugzilla.gnome.org/show_bug.cgi?id=742643


VLC was tricking me by only accepting mono or stereo streams. Anything else will be considered a corrupt stream. Eventhough it isn't.

Now I am going to look into supporting demuxing of Opus in qtdemux.

After that I will revisit the possibility of having to shift timestamps based on the pre_skip.

commit a400d504ca25ba3812a563a4cac94ef2bf910b2a
Author: Luis de Bethencourt <luisbg@osg.samsung.com>
Date:   Thu Nov 26 21:46:11 2015 +0000

    qtdemux: add support for Opus

    Add support for demuxing Opus encapsulated in MP4 files, based on the
    following spec: https://www.opus-codec.org/docs/opus_in_isobmff.html

    https://bugzilla.gnome.org/show_bug.cgi?id=742643

Now we can mux and demux.

Demuxer work has been tested in mono, stereo and 5.1 files.

Remaining work is to revisit the possibility of having to shift timestamps based on the pre_skip.

And actual *handling* of pre-skip and the end padding in the demuxer and muxer, that is putting the GstAudioClippingMeta on buffers or reading it from buffers.

If you need to shift timestamps or not is a separate issue to this.

Roger that.

Thanks for the help Sebastian :)

Will be on this very soon.

What's the progress here?

Hi Sebastian.

Sorry, urgent things got in the way of important ones. Will have time to put the GstAudioClippingMeta in the next few days for sure.

The spec [0] defines that:
"Due to the priming samples (or the padding at the beginning) derived from the pre-roll for the startup and the padded samples at the end, we need trim from media to get the actual duration. An edit in the Edit List Box can achieve this demand, and the Edit Box and the Edit List Box shall be present."

In other words, the pre_skip should be the media_start value of the elst_entry.

Which is set in gst_qt_mux_update_edit_lists () [0], which if qtpad->dts_adjustment is > 0, passes (qtpad->first_ts - qtpad->first_dts) as media_start.

Now, the question is how do I make the pre_skip, gathered in gst_qt_mux_audio_sink_set_caps(), available in gst_qt_mux_add_buffer() to be able to set dts_adjustment and first_dts in?

[0] http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/isomp4/gstqtmux.c#n2448

Let's reassign to the default assignee then, there does not seem to be any progress 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-good/issues/150.