GNOME Bugzilla – Bug 768763
vorbisenc, opusenc reconfiguration broken
Last modified: 2016-10-31 14:33:35 UTC
This reverts commit a16cd5d2a5cbdf084163ead68b59d537d7db99f7: "vorbisenc: push an updated segment stop time when we know it"
Created attachment 331392 [details] [review] Revert "vorbisenc: push an updated segment stop time when we know it" This reverts commit a16cd5d2a5cbdf084163ead68b59d537d7db99f7. Setting the stop time on the segment breaks reconfiguration, as the encoder signals an EOS, but we reconfigure it an continue to produce buffers. This information should not be required via the segment downstream since we already have the sample count being used to generate buffer durations.
cc'ing Vincent to double check.
Regression presumably, so marking as blocker. Do we have a test case for this by any chance Arun?
Created attachment 331450 [details] Test case for vorbisenc renegotiation Attaching a test case. Run it, and you'll see output from fakesink stop after 2 seconds (when caps change). Works with my patch.
Seems OK to rely on buffer durations. If that breaks something, we can always fix it another way. Though ideally, having an end time breaking downstream seems wrong in some way. Does it work with segment seeks ?
It breaks because the end time is incorrect w.r.t. to the stream. What's happening is, to a first approximation): [buffer ts=1 dur=1] [buffer ts=2 dur=1] [caps change => new segment stop=2+1=3] [buffer ts=3 dur=1] [buffer ts=4 dur=1] The third buffer onwards are of different caps from the first two. The timestamps, though, continue as normal. But the segment stop is from wherever buffer 2 ends, so subsequent buffers get dropped.
I've pushed out the revert now. Leaving this open for a few things: 1. Might be nice to have the test case code as a make check test 2. Not sure what the original commit was trying to do, so there might be some other case we still need to deal with 3. Would be good to have this in the 1.8 branch
I think without further changes, this breaks stuff. This is needed for gapless muxing AFAIU, and you will otherwise need to use the new API used by opusenc.
Turns out we need to deal with the same thing on opusenc as well (same test, s/vorbis/opus/ shows no buffers after caps change).
So what should we do here? I can cook up a patch to not throw out segment stops in opusenc as well, but how do we not break gapless muxing?
It does in Ogg, yes. The solution would be to use the new audio clipping meta in both vorbisenc and oggmux for this instead of breaking the segment.
The Vorbis case was a bit annoying, as I get a granulepos that corresponds to the end, but there's no otherwise known duration of the "full" buffer to remove from, so I resized the buffer to match the granpos instead.
Created attachment 336275 [details] [review] vorbisenc: strip after eos samples from the end of the buffer
Created attachment 336276 [details] [review] oggmux: take audio clip meta into account
Need to change the same for opusenc
Comment on attachment 336275 [details] [review] vorbisenc: strip after eos samples from the end of the buffer This looks wrong. You should still output the whole *compressed* packet, and not just do some random calculation by using the *uncompressed* size
Review of attachment 336276 [details] [review]: ::: ext/ogg/gstoggmux.c @@ +876,3 @@ + if (meta->format == GST_FORMAT_DEFAULT) { + if (meta->end > duration) { + g_assert_not_reached (); Maybe clip here instead? @@ +881,3 @@ + } + } else { + g_assert_not_reached (); Maybe without this landmine :) Make it a GST_WARNING and don't clip
Created attachment 336316 [details] [review] vorbisenc: strip after eos samples from the end of the buffer Oops, way too used to decoders :)
Created attachment 336317 [details] [review] oggmux: take audio clip meta into account
(In reply to Sebastian Dröge (slomo) from comment #15) > Need to change the same for opusenc opusenc already uses the clip meta.
(In reply to Vincent Penquerc'h from comment #20) > (In reply to Sebastian Dröge (slomo) from comment #15) > > Need to change the same for opusenc > > opusenc already uses the clip meta. But it still modifies the segment, which is the main problem here
Comment on attachment 336316 [details] [review] vorbisenc: strip after eos samples from the end of the buffer Now you only need to actually add the clip meta to the buffer :)
(In reply to Sebastian Dröge (slomo) from comment #22) > Comment on attachment 336316 [details] [review] [review] > vorbisenc: strip after eos samples from the end of the buffer > > Now you only need to actually add the clip meta to the buffer :) I don't use a clip meta here, I just use the buffer duration to control the amount to clip. This is because I don't know the actual duration of the whole buffer it's returned, I just know how much there should be (from op.granulepos), so I don't know how much samples to clip.
(In reply to Vincent Penquerc'h from comment #23) > (In reply to Sebastian Dröge (slomo) from comment #22) > > Comment on attachment 336316 [details] [review] [review] [review] > > vorbisenc: strip after eos samples from the end of the buffer > > > > Now you only need to actually add the clip meta to the buffer :) > > I don't use a clip meta here, I just use the buffer duration to control the > amount to clip. This is because I don't know the actual duration of the > whole buffer it's returned, I just know how much there should be (from > op.granulepos), so I don't know how much samples to clip. Why not? You know how many samples there are in this frame (including padding)? Without adding the clip meta here, this is rather useless.
Created attachment 336318 [details] [review] opusenc: remove segment stop modification on eos
(In reply to Sebastian Dröge (slomo) from comment #24) > Why not? You know how many samples there are in this frame (including > padding)? I did not see it, but I will have another look. This is in a loop, where vorbisenc gives us packets, so theoretically it can split however it wants. > Without adding the clip meta here, this is rather useless. I don't get why. The clip meta will modify the duration by trim_end, so it seems like a similar operation here.
(In reply to Vincent Penquerc'h from comment #26) > (In reply to Sebastian Dröge (slomo) from comment #24) > > Why not? You know how many samples there are in this frame (including > > padding)? > > I did not see it, but I will have another look. This is in a loop, where > vorbisenc gives us packets, so theoretically it can split however it wants. Vorbis has fixed length packets, and you can look at the packet to know the number of samples. > > Without adding the clip meta here, this is rather useless. > > I don't get why. The clip meta will modify the duration by trim_end, so it > seems like a similar operation here. You mean that oggmux would clip by using the duration instead of the clip meta? The problem then only is that the duration includes rounding errors... and it won't work well for other muxers.
(In reply to Sebastian Dröge (slomo) from comment #27) > Vorbis has fixed length packets, and you can look at the packet to know the > number of samples. At the risk of missing something obvious, is there something simpler than gst_parse_vorbis_setup_packet on the codebooks, then packet_duration_vorbis on actual packets ?
Sadly not, as far as I know (and for extra fun the info needed is at the very end of the setup data so no shortcuts).
Created attachment 336323 [details] [review] vorbisenc: strip after eos samples from the end of the buffer
Review of attachment 336323 [details] [review]: ::: ext/vorbis/gstvorbisenc.c @@ +982,3 @@ + "Adding trim-end %" G_GUINT64_FORMAT, trim_end); + gst_buffer_add_audio_clipping_meta (buf, GST_FORMAT_DEFAULT, 0, + trim_end); Looks good but is there also trimming at the very beginning needed maybe?
I don't understand how this might make sense, since we're looking at the end granule to see if any samples have been skipped. If you mean that some other part of the code might want to trim samples at th beginning for another reason, then maybe. Can there be just one clipping meta on a buffer ?
The very first output packet might have some padding at the beginning that should be clipped. Usually audio codecs do that.
It might be, but that does not seem relevant here, since this is about adjusting the end time precisely. Could it have an influence somehow ?
Yes, it prevents gapless decoding, which is the whole point of adjusting the duration of the last packet.
Ah, right. I think Vorbis allows that too, I'll go check spec probably next week.
I *think* that it doesn't need clipping at the beginning, since the encoder will yield a packet that starts with the first raw sample. This is not like the decoder, which needs clipping based on the first granule. I admit I'm not super sure about this. Also, can there be a clip meta on incoming buffers that every element would have to also honour ? So we'd need to intersect clip start/stop, so the output buffer ends up with the strictest clipping ?
(In reply to Vincent Penquerc'h from comment #37) > I *think* that it doesn't need clipping at the beginning, since the encoder > will yield a packet that starts with the first raw sample. This is not like > the decoder, which needs clipping based on the first granule. I admit I'm > not super sure about this. That would be surprising. Usually encoders have some kind of "run in" at the beginning, unless this is automatically handled by the decoder in which case we wouldn't have to care. If you decode the first Vorbis frame, does it start with the beginning or does it first have some silence/noise? > Also, can there be a clip meta on incoming buffers that every element would > have to also honour ? So we'd need to intersect clip start/stop, so the > output buffer ends up with the strictest clipping ? I don't understand your question :)
There's some automatic accounting for this (end of 1.3.2 in doc/index.html in libvorbis): > Data is not returned from the first frame; it must be used to ’prime’ the decode engine. The encoder > accounts for this priming when calculating PCM offsets; after the first frame, the proper PCM output > offset is ’0’ (as no data has been returned yet). I don't think there's anything else to do about this. As for my question about incoming buffers, it is: if I get a raw PCM buffer of 100 samples, and there is a clip meta on it with start=20 and end=20, those presumably need to be applied to the outgoing buffer as well, right (and, with this patch, the end needs to be set to max(20, X) if X is the result of the calculation in this patch) ?
(In reply to Vincent Penquerc'h from comment #39) > There's some automatic accounting for this (end of 1.3.2 in doc/index.html > in libvorbis): > > > Data is not returned from the first frame; it must be used to ’prime’ the decode engine. The encoder > > accounts for this priming when calculating PCM offsets; after the first frame, the proper PCM output > > offset is ’0’ (as no data has been returned yet). > > I don't think there's anything else to do about this. So it seems, simpler than Opus then ;) > As for my question about incoming buffers, it is: if I get a raw PCM buffer > of 100 samples, and there is a clip meta on it with start=20 and end=20, > those presumably need to be applied to the outgoing buffer as well, right > (and, with this patch, the end needs to be set to max(20, X) if X is the > result of the calculation in this patch) ? clip meta is only really supposed to be on encoded data, and it is applied/consumed after decoding. It makes no sense on raw PCM buffers, adding a meta is more work than just creating a subbuffer.
(In reply to Vincent Penquerc'h from comment #39) > There's some automatic accounting for this (end of 1.3.2 in doc/index.html > in libvorbis): End of 1.3.2 in doc/Vorbis_I_spec.html of course.
Comment on attachment 336323 [details] [review] vorbisenc: strip after eos samples from the end of the buffer Let's go ahead with this then
commit 9599c3416ca907a4201765230aa0786fd8fb861a Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Sep 27 09:24:08 2016 +0100 opusenc: remove segment stop modification on eos https://bugzilla.gnome.org/show_bug.cgi?id=768763 commit a30c713e43823390889ea847531785a9253ecaaa Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 26 16:31:06 2016 +0100 oggmux: take audio clip meta into account for buffer duration https://bugzilla.gnome.org/show_bug.cgi?id=768763 commit 1a4ba790446ef0e043bec9cd9d78aa296235d47b Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 26 16:25:14 2016 +0100 vorbisenc: strip after-eos samples from the end of the eos buffer https://bugzilla.gnome.org/show_bug.cgi?id=768763
Created attachment 337160 [details] [review] fix codebooks packet identifier "oops"