View Bug Details

IDProjectCategoryView StatusLast Update
0001771DCP-o-maticBugspublic2023-11-20 16:47
Reportercarl Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status confirmedResolutionopen 
Summary0001771: Audio artefacts at ends of audio files
Description

WAVs come from a post house split into reels, but prepared for 23.976. Then DCP-o-matic resamples them and there are discontinuities when one resampler is flushed and the next started.

  • concatenate the files before FFmpeg processes them? looks hard
  • re-use resamplers

When AudioDecoder makes a resampler we could re-use one from the same channel/samplerate/correct timing. If we had content

A1 A2

A1 would need to not flush its resamplers, then A2 would re-use them. A1 needs to know it has something following it.

Maybe some ResamplerManager that is set up in Player::setup_pieces. It knows which AudioDecoder is using what Resamplers and provides them on request to the AudioDecoder. The decoder can also tell the manager that it might want to flush the resamplers.

This was going fine in 1771-resample-glitches until ResamplerManager::maybe_flush; here it seems that we'll run into problems because Player may throw away samples that don't appear to come from the right place.

TagsNo tags attached.
Branch
Estimated weeks required
Estimated work requiredMajor

Activities

carl

2020-06-23 21:03

administrator   ~0003848

Last edited: 2020-07-05 23:35

Another idea might be

Extend Piece so that it has

vector<Content>
vector<Decoder>

where there are one or more of each, 1 decoder per 1 content.

Then extend the Piece API so that it does everything currently done by calls into the content from Player.

Content would be:

  • all the same type
  • only with start trim on 1st content and end trim on last
  • some settings the same

Tricky bits with this are:

  • _stream_states and audio streams in general
  • re-use of old decoders (optimisation)

_stream_states are piece, last_push_end (DCPTime) for every stream in every piece of content Initialised to content->position() in setup_pieces_unlocked

last_push_end updated when audio is pushed into the merger; here the piece and stream are known. Checked when deciding the time before which we have all audio

_stream_states could be a vector in Piece, then it is managed by Piece; call it when audio is pushed, it finds the correct stream from its internal list of content, then updates. Then you could ask it what the earliest stream is.

Going reasonably OK in 1771-resamples-glitches-take3

carl

2021-04-22 22:38

administrator   ~0004287

Probably easiest to manually rebase -take3 onto v2.15.x to remember what is going on.

carl

2021-04-22 23:32

administrator   ~0004291

Made -take4 branch but didn't do anything yet.

carl

2021-04-24 23:07

administrator   ~0004296

Did some work in -take4 but this is starting to feel like a very long way round; even setting Piece up to have multiple content takes a lot, and that is before even setting up the Pieces to take advantage of this.

carl

2021-04-26 23:53

administrator   ~0004300

Last edited: 2021-04-26 23:55

The AudioDecoder has a Resampler. Say you have two pieces of content that are being resampled but should connect seamlessly:

/-------\/-----\
| 1     | 2    |
\-------/\-----/

When 1's decoder finishes it flushes its resampler, 2 makes a new one. This causes a slight discontinuity.

==> move the Resampler somewhere else (e.g. the player) - it used to be like this until fa4d252

==> keep the same resampler

When 1 finishes maybe it doesn't flush its resampler. Without the flush we actually emit audio up to a bit before the end of 1, then start decoding 2 and pushing its samples into the same resampler. Then stuff comes out of the resampler and this stuff should be at a time just before 2 starts, so the player discards it (since it's outside the bounds of content 2).

==> move this discarding of data back a bit

Player::audio's data comes from AudioDecoder::Data emitting - so can't we stop that emitting out-of-bounds data? But the data would also be out of bounds as far as AudioDecoder is concerned, because it only knows about the one piece of content.

==> somehow coalesce the content or make it look coalesced

take4 does this by changing Piece to allow multiple content with a single "special" decoder which coalesces the output of multiple "real" decoders so that their output ends up in a single AudioDecoder and hence a
single resampler. But this "special" decoder is awkward as it has to present the Decoder interface and proxy things down to sub-decoders.

===> coalesce the content at a different level, e.g. make the existing decoder able to take multiple files
as input

...but you need a decoder for each one to discard the headers and get to the actual samples.
...and Piece is the obvious place to coalesce, since this is a data structure set up just for playing,
invisible to the user.

To maintain the Player::audio semantics of "discard outside Piece::{position,end}" Piece must have multiple bits of content. But Player also decides what to pass() based on Pieces, so that means the piece must manage multiple decoders (or have a proxy decoder which does).

The real question perhaps is, even after this all this preparation, where will the Resampler end up?

===> just have one AudioDecoder shared between multiple (e.g.) FFmpegDecoders?

But Resamplers are managed one-per-stream; when the AD sees data from a stream it hasn't seen before it makes a Resampler.

===> Move Resampler into Piece?

Piece would need _positions. Maybe workable?

===> Just noticed the stuff in FFmpegDecoder::flush to round lengths up to the next video frame and so on. Then the resamplers are flushed... dubious... but maybe OK (the video frame round-up will have to go).

AudioDecoder would not resample; would store stream positions in content frames.
Piece would probably have to have an Audio emission that the player attaches to, and intercept the one from AudioDecoder, resample it with one resampler and re-emit.

Piece soon becomes a sort of decoder; e.g. when the last of its decoders flushes it needs to flush the resamplers; Player must listen to Piece::Audio for audio data rather than decoder->audio->Data;
suggests going back to the grouped-decoder class, but then you need some AudioDecoders that have resamplers and some that do not.

Though in fact it's probably neater if Player listents to Piece::Video, Piece::Text etc., so that might be OK. Piece becomes some content with some decoders merging that content and presenting
data to Player. Maybe Content{Video,Audio,Text} becomes Piece{Video,Audio,Text} and decoders just emit their data without a wrapper.

carl

2021-05-05 22:48

administrator   ~0004307

Last edited: 2021-05-06 00:17

More and more this seems like a crazy amount of work for a relatively small bug; to trigger it requires not only post-production at the wrong frame rate (as if the content were not intended for DCP) but also in reels (as if it were for DCP).

There's also a high risk of things breaking as a result.

Pros of merging before 2.16.0:

  • might get more testing
  • it will get done rather than bit-rotting
  • there are perhaps some nice fixes / structural improvements as part of the patch
  • it will be easier to rebase 2.17.x onto 2.16.x in the early part of development

Cons:

  • it might break things
  • it's taking ages
  • merging it onto 2.17.x will make the branches diverge early

Although if the idea is to release 2.18.0 soon it may not be so bad if there's a divergence.

carl

2021-05-06 23:27

administrator   ~0004310

Last edited: 2021-05-06 23:27

State as of now is that most tests pass; one notable exception is content_test6.
This is the one which checks how we handle large audio timestamp discontinuities. AudioDecoder resets its position if the timestamp coming from the decoder is more than 1 video frame away from where it "should be". Now, though, that reset is passed onto the Piece but then ignored.

It seems like we should do the stuff in AudioDecoder::emit in Piece instead. And maybe that implies that AudioDecoder should not keep _position at all, and instead of Piece::decoder_before calling out to the decoders to ask what is going on, it should know itself;
for audio it is already counting position in the streams and perhaps it should do the same for video and subs. Or maybe we could special-case audio and do that ourselves.

carl

2021-05-06 23:28

administrator   ~0004311

Either way this definitely feels now like far too much for 2.16.0.

Bug History

Date Modified Username Field Change
2020-06-20 21:40 carl New Bug
2020-06-20 21:44 carl Description Updated
2020-06-20 21:44 carl Estimated work required => Undecided
2020-06-21 20:09 carl Description Updated
2020-06-23 21:03 carl Note Added: 0003848
2020-06-23 21:03 carl Note Edited: 0003848
2020-06-23 23:27 carl Note Edited: 0003848
2020-07-05 23:34 carl Status new => confirmed
2020-07-05 23:35 carl Note Edited: 0003848
2020-08-18 23:29 carl Estimated work required Undecided => Major
2021-04-22 21:31 carl Tag Attached: alpha-1-blocker
2021-04-22 22:38 carl Note Added: 0004287
2021-04-22 23:32 carl Note Added: 0004291
2021-04-24 23:07 carl Note Added: 0004296
2021-04-26 23:53 carl Note Added: 0004300
2021-04-26 23:53 carl Note Edited: 0004300
2021-04-26 23:54 carl Note Edited: 0004300
2021-04-26 23:54 carl Note Edited: 0004300
2021-04-26 23:55 carl Note Edited: 0004300
2021-04-26 23:55 carl Note Edited: 0004300
2021-05-05 22:48 carl Note Added: 0004307
2021-05-06 00:17 carl Note Edited: 0004307
2021-05-06 23:27 carl Note Added: 0004310
2021-05-06 23:27 carl Note Edited: 0004310
2021-05-06 23:28 carl Note Added: 0004311
2021-05-06 23:28 carl Target Version 2.16.0 => 2.18.0
2021-05-06 23:28 carl Tag Detached: alpha-1-blocker
2022-09-10 21:39 carl Target Version 2.18.0 =>
2023-09-26 22:15 carl Priority immediate => normal