Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Master Feature) Add --retain-markers to rdimport(1) and dropboxes #383

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

(Master Feature) Add --retain-markers to rdimport(1) and dropboxes #383

wants to merge 19 commits into from

Conversation

deltecent
Copy link
Contributor

@deltecent deltecent commented Feb 26, 2019

Adds the ability for rdimport(1) and dropboxes to import new audio to an existing cart without overwriting previous markers.

I'm not sure how to handle multiple PRs with database changes, so I'm making them all the next number, 307.

@ElvishArtisan
Copy link
Owner

What happens if multiple cuts exist when using --retain-markers --delete-cuts? Which cut's markers get retained?

@ElvishArtisan
Copy link
Owner

The Cut Start and Cut End markers are not preserved when using the following command line:
rdimport --verbose --to-cart=999997 --metadata-pattern=%t_%a. --delete-cuts --retain-markers --autotrim-level=0 TEST Ocean\ Eyes_Infrared\ Rabbit.wav .

Perhaps it would be useful to retain them if Auto Trim is disabled (--autotrim-level=0)?

@deltecent
Copy link
Contributor Author

Another good question. My first thought was, this could/should only happen once, since all the cuts will be deleted after the first run. I choose to use the meta data from the next scheduled cut using RDCart::selectCut(). I know what I will be doing is let the dropbox load the cut and then edit the markers since any cuts from existing dropboxes will not have any markers.

@deltecent
Copy link
Contributor Author

The Cut Start and Cut End markers are not preserved when using the following command line:
rdimport --verbose --to-cart=999997 --metadata-pattern=%t_%a. --delete-cuts --retain-markers --autotrim-level=0 TEST Ocean\ Eyes_Infrared\ Rabbit.wav .

Perhaps it would be useful to retain them if Auto Trim is disabled (--autotrim-level=0)?

Yes, that info should have been retained. I'll get that fixed per your recommendation.

@ElvishArtisan
Copy link
Owner

Is RDCart::selectCut() really appropriate for this case? It can produce some "strange" results. For example, say you have one existing cut, which has expired. RDCart::selectCut() will then return no cut, with the result no markers at all will be preserved.

I'm thinking that perhaps we need a simple rule for this that is not dependent cut dayparting --e.g. always use the markers from the newest (most recently imported) cut.

@deltecent
Copy link
Contributor Author

That's an option. I couldn't really think of which was the best way to handle multiple cuts was. I was considering logging an error/warning if there was more than 1 cut. If there's more than 1 cut we either have to make a best guess (which will probably be wrong) or skip it and let the user remove the extra cuts.

It's really a chicken/egg problem of which came first, the cart or the dropbox?

Also, speaking of chicken/egg, now that the PR with database 307 has been merged, I'll update the this PR to 308.

@ElvishArtisan
Copy link
Owner

That's an option. I couldn't really think of which was the best way to handle multiple cuts was. I was
considering logging an error/warning if there was more than 1 cut. If there's more than 1 cut we
either have to make a best guess (which will probably be wrong)

Make a best guess --e.g. 'use markers from the most recently imported cut'-- and make sure that that's documented clearly in the man page. That way, everyone knows what to expect.

or skip it and let the user remove the extra cuts.

If --delete-cuts has been specified, then that's what should happen, regardless. The sole question to decide here is: which cut's markers should we apply to the new cut.

@deltecent
Copy link
Contributor Author

Interesting... I made the --retain-markers only work if --delete-cuts was specified in rdadmin(1). I didn't even think of it working with multiple cuts.

So the plan is to use markers from the most recently imported cut unless --delete-cuts is specified in which case log an error if there are multiple cuts.

One other question while we're on the subject, does --delete-cuts make any sense if a specific cart isn't specified for the dropbox?

@ElvishArtisan
Copy link
Owner

Interesting... I made the --retain-markers only work if --delete-cuts was specified in rdadmin(1). I
didn't even think of it working with multiple cuts.

I think we're operating on a misunderstanding here. Suppose there are multiple cuts in the targeted cart before any cuts are deleted? That was the case I was asking about.

So the plan is to use markers from the most recently imported cut unless --delete-cuts is specified in
which case log an error if there are multiple cuts.

Why would that be an error? Simply use the same rule (apply markers from most-recently-imported cut).

One other question while we're on the subject, does --delete-cuts make any sense if a specific cart
isn't specified for the dropbox?

No, because in that case, a new cart is created, which is by definition empty initially. So actually, the --retain-markers switch makes sense only if --to-cart is also given.

@deltecent
Copy link
Contributor Author

Ok. I think I have it now. PR updated.

@ElvishArtisan
Copy link
Owner

Alas, yet another issue...

Create a cart, import a long-ish piece of audio into it (say, 4:00 long). Set the segue markers out near the end (say, around 3:58 or so).

Now, do rdimport --delete-cuts --to-cart=<cartnum> --retain-markers, but using a shorter (say, 3:00 long) piece of audio. The markers are indeed retained, but are now invalid because they are placed beyond the end of the actual audio.

We need some sanity checks here. If just an end marker is beyond the end of the imported audio, move it back to the end audio position. If both the start and end markers are beyond the end, then remove both markers (with the exception of 'Cut Start' and 'Cut End', in which case the best we can do is put them at the start and end of the actual audio). Some warnings to stderr and/or syslog would probably be a Good Idea too.

@deltecent
Copy link
Contributor Author

This feature was based on the assumption that the length of audio imported was constant.

If that isn't a valid assumption then we have to do something along the lines of what you recommend.

If it is a valid assumption, then I feel the thing to do is retain the markers as-is and log a warning.

@ElvishArtisan
Copy link
Owner

Assumption or not, it's something that's going to be broken sooner or later; through user ignorance, malice or simple inadvertance. When that happens, we have to be able to cope with it.

@deltecent
Copy link
Contributor Author

What happens when the log player bumps into a cut where the markers are out of bounds? Maybe we should handle out of bounds markers there and not modify them in the database?

@ElvishArtisan
Copy link
Owner

No cut that exists within Rivendell should have 'out-of-bounds' marker data. This is one of the basic invariants of the database design. Any process that imports 'external' marker data --e.g. rdimport processing timer data from a CartChunk -- must perform validation on that data to ensure that no 'out-of-bounds' conditions exist before writing such to the database. You can see an example of how such validation works in the RDCut::setMetadata() method.

The situation here of course is 'backwards' from the usual case -- it's not the marker data that is changing, but rather the underlying audio to which the marker data is being applied.

@deltecent
Copy link
Contributor Author

Sounds good. I'm thinking about back-peddling on this feature since it seems like any tremor in the force will cause bad things to happen. I'll put the validation in and see how it looks.

@deltecent
Copy link
Contributor Author

I have not tested this yet, but if you could look at RDCut::validateMarkers and see if I'm on the right track...

Also, what do you think about a method to load all the markers into variables with a single SQL statement and another method to write them all with a single SQL statement. The way all of these markers are handled seems expensive.

@ElvishArtisan
Copy link
Owner

ElvishArtisan commented Mar 5, 2019

Also, what do you think about a method to load all the markers into variables with a single SQL
statement and another method to write them all with a single SQL statement. The way all of these
markers are handled seems expensive.

That's putting it quite charitably. :)

Yes, the whole business of marker validation grew by accretion over time, which is the major reason why it's the diffuse mess that it is right now. I agree that centralizing this validation logic is a Good Idea. I'm thinking of something along the lines of:

bool RDWaveData::validateMarkers(int len=-1)

Where len is the length (in mS) of the underlying audio to use when performing the validation. Omitting that parameter would cause the method use the cut's current length as determined by RD WavaData::length(). A return value of true would indicate that one or more modifications were made to the markers to make them valid.

You can populate an instance of RDWaveData with a particular cut's data with the (already existing) RDCut::getMetadata() method. Likewise, modified results can be written back using RDCut::setMetadata().

My goal is to have the new RDWaveData::validateMarkers() method ready for testing today. I'll drop you a line when I've committed it.

@deltecent
Copy link
Contributor Author

My goal is to have the new RDWaveData::validateMarkers() method ready for testing today. I'll drop you a line when I've committed it.

That sounds like a great idea!

@ElvishArtisan
Copy link
Owner

Committed in [master d024d27].

At this moment, RDWaveData::validateMarkers() is not actually being used anywhere. My goal is eventually to have all marker validation (starting with the mess in RDCut::setMetadata()) simply call that method.

@deltecent
Copy link
Contributor Author

I still have more work to do. Please ignore these latest commits.

@deltecent
Copy link
Contributor Author

I've run into a problem that I may not have time to look at for a couple of days. The problem may be with the audio editor in rdlibrary(1), not with this PR, so I'm going to push the changes for this PR.

I'm using the test tone cart for testing. When that cart is imported with rdimport(1), it says the length of the cut is 10069.

07/03/2019 - 10:18:13.659 : Retaining markers from cart 040048 cut 001
07/03/2019 - 10:18:13.660 : Deleting cuts from cart 040048
07/03/2019 - 10:18:13.708 : Importing file "999999_001.wav" to cart 040048 ...
07/03/2019 - 10:18:16.136 : Import Done.
07/03/2019 - 10:18:16.138 : Restoring markers for cart 040048_001
07/03/2019 - 10:18:16.138 : length=10069
07/03/2019 - 10:18:16.138 : startPos=0
07/03/2019 - 10:18:16.138 : endPos=10069
07/03/2019 - 10:18:16.138 : talkStartPos=-1
07/03/2019 - 10:18:16.139 : talkEndPos=-1
07/03/2019 - 10:18:16.139 : segueStartPos=-1
07/03/2019 - 10:18:16.139 : segueEndPos=-1
07/03/2019 - 10:18:16.139 : hookStartPos=-1
07/03/2019 - 10:18:16.139 : hookEndPos=-1
07/03/2019 - 10:18:16.139 : fadeUpPos=-1
07/03/2019 - 10:18:16.139 : fadeDownPos=-1
07/03/2019 - 10:18:16.141 : Deleted source file "999999_001.wav"

I then go into "Edit Markers" and do nothing but click "Save". When I import the same audio file, the end marker has been changed from 10069 to 10082 causing an out-of-bounds return from validateMarkers() since the end marker is now beyond the length.

07/03/2019 - 10:19:01.146 : Retaining markers from cart 040048 cut 001
07/03/2019 - 10:19:01.147 : Deleting cuts from cart 040048
07/03/2019 - 10:19:01.195 : Importing file "999999_001.wav" to cart 040048 ...
07/03/2019 - 10:19:01.587 : Import Done.
07/03/2019 - 10:19:01.588 : Restoring markers for cart 040048_001
07/03/2019 - 10:19:01.588 : length=10069
07/03/2019 - 10:19:01.588 : startPos=0
07/03/2019 - 10:19:01.588 : endPos=10082
07/03/2019 - 10:19:01.588 : talkStartPos=-1
07/03/2019 - 10:19:01.588 : talkEndPos=-1
07/03/2019 - 10:19:01.588 : segueStartPos=-1
07/03/2019 - 10:19:01.588 : segueEndPos=-1
07/03/2019 - 10:19:01.588 : hookStartPos=-1
07/03/2019 - 10:19:01.588 : hookEndPos=-1
07/03/2019 - 10:19:01.588 : fadeUpPos=-1
07/03/2019 - 10:19:01.589 : fadeDownPos=-1
07/03/2019 - 10:19:01.589 : Out-of-bounds detected. Markers changed for cart 040048_001
07/03/2019 - 10:19:01.591 : Deleted source file "999999_001.wav"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants