-
Notifications
You must be signed in to change notification settings - Fork 80
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
Added enhanced Multidisc dropout correction feature to ld-dropout-correct #393
Conversation
…ultiple source to the dropout correct class
…class ready for multisource
@@ -44,32 +44,32 @@ bool CorrectorPool::process() | |||
if (!targetVideo.open(stdout, QIODevice::WriteOnly)) { | |||
// Could not open stdout | |||
qInfo() << "Unable to open stdout"; | |||
sourceVideos[0].close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing one of the source videos doesn't sound right (and similarly below). It would probably be better for the caller to be responsible for closing the videos, since it provided them in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
for (qint32 sourceNo = 0; sourceNo < numberOfSources; sourceNo++) { | ||
// Determine the fields for the input frame | ||
if (sourceNo == 0) { | ||
// No need to perform VBI frame number mapping on the first source | ||
firstFieldNumber[sourceNo] = ldDecodeMetaData[0].getFirstFieldNumber(frameNumber); | ||
secondFieldNumber[sourceNo] = ldDecodeMetaData[0].getSecondFieldNumber(frameNumber); | ||
firstFieldNumber[sourceNo] = ldDecodeMetaData[sourceNo]->getFirstFieldNumber(frameNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks the same as the block below.
Restructure as if (sourceNo != 0 && currentVbiFrame >= ...) { set to -1; } else { this block of code }?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not quite the same, but I restructured it a bit to make it clearer
@@ -75,54 +90,71 @@ void DropOutCorrect::run() | |||
" drop-outs"; | |||
|
|||
// Analyse the drop out locations in the first field | |||
QVector<DropOutLocation> firstFieldDropouts; | |||
if (firstFieldMetadata[0].dropOuts.startx.size() > 0) firstFieldDropouts = setDropOutLocations(populateDropoutsVector(firstFieldMetadata[0], overCorrect)); | |||
QVector<QVector<DropOutLocation>> firstFieldDropouts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can give QVector's constructor the size directly (as with std::vector), i.e. rather than writing
QVector<T> foo;
foo.resize(n);
just write
QVector<T> foo(n);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
void DropOutCorrect::correctField(const QVector<QVector<DropOutLocation>> &thisFieldDropouts, | ||
const QVector<QVector<DropOutLocation>> &otherFieldDropouts, | ||
QVector<QByteArray> &thisFieldData, const QVector<QByteArray> &otherFieldData, | ||
bool thisFieldIsFirst, bool intraField, QVector<qint32> availableSourcesForFrame, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const QVector<...> &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
qint32 dropOutIndex, bool thisFieldIsFirst, bool matchChromaPhase, | ||
bool isColourBurst, bool intraField) | ||
bool isColourBurst, bool intraField, QVector<qint32> availableSourcesForFrame, | ||
QVector<qreal> sourceFrameQuality) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const QVector<...> &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
// If no candidate is found, return no replacement | ||
Replacement replacement; | ||
replacement.distance = 0; | ||
|
||
if (!candidates.empty()) { | ||
// Find the candidate with the lowest spatial distance from the dropout | ||
qint32 lowestDistance = 1000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're storing the lowest distance as replacement.distance now, so lowestDistance can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (sourceNo == 0) sourceLine += sourceOffset; | ||
|
||
// Is the line within the active range? | ||
if (sourceLine < firstActiveFieldLine || sourceLine > lastActiveFieldLine) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>=
rather than >
? (For consistency with < lastActiveFieldLine
below.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
+ ((dropOut.fieldLine - 1) * videoParameters[0].fieldWidth); | ||
|
||
if (chromaReplacement.fieldLine == -1) { | ||
// Choose whole signal or just chroma replacement | ||
// Never use chroma if the source of the replacement is > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It could still be coming from a different line in another source, so there's still a benefit in using separate chroma/luma sources.
Maybe if (chromaReplacement.fieldLine == -1 || (dropOut.fieldLine == replacement.fieldLine) && (dropOut.fieldLine == chromaReplacement.fieldLine))
(i.e. don't bother filtering if it's coming from another source's copy of the same line)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
sourceVideos.resize(totalNumberOfInputFiles); | ||
for (qint32 i = 0; i < totalNumberOfInputFiles; i++) { | ||
// Create an object for the source video | ||
sourceVideos[i] = new SourceVideo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
these at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ldDecodeMetaData.resize(totalNumberOfInputFiles); | ||
for (qint32 i = 0; i < totalNumberOfInputFiles; i++) { | ||
// Create an object for the source video | ||
ldDecodeMetaData[i] = new LdDecodeMetaData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
these at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - cheers!
Multidisc dropout correction along with some light updates to the ld-decode-tools library. This version of ld-dropout-correct can correct a TBC using up to 31 additional sources for the correction data.