From 2aa921b21573ce72e9a507ec298f2d2ebe217dec Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Fri, 17 Jul 2026 08:26:48 +0800 Subject: [PATCH] fix: bug-fix sweep across node, audio, render, plugin subsystems Node core: - MathNode/TrigonometryNode combo strings realigned with Operation enums - mathbase scalar/vector operand pick no longer uses bitwise type checks - NodeSetPositionAndDependenciesRecursively moves dependencies again - RemoveAllKeyframes undo actually restores keyframes - NodeGroup GetInputName null-deref guard, passthrough ids use input id - NodeValueTable::Has is an exact type match; tag fallback only for empty tags; kStrCombo/kPushButton get data type names - delete_all_keyframes no longer loops forever on unparented keyframes; keyframe-load failures propagate; rational interpolation falls back to double; OpacityEffect no longer leaks its internal MathNode Audio/footage: - AudioVisualWaveform: GetSummaryFromTime underflow OOB read, TrimIn prepend length bookkeeping, OverwriteSums source channel indexing - PanNode inserts the pan value into the sample job (keyframed pan works); OutputParamsChanged is emitted on device change; PortAudio device indices are validated before Pa_GetDeviceInfo - Footage: AdjustTimeByLoopMode no longer hangs/UBs on degenerate lengths, GetStreamIndex bounds-checked, CheckFootage clears stale state on missing files, failed probes are not cached, FootageDescription::Load requires its own root element Render/track: - ViewerOutput pushes the tagged samples value; TrackList disconnects the track-height lambda; GetTrackFromReference validity check - RenderManager dummy backend: null-initialized threads, guarded decoder-cache/timer paths; Renderer::Destroy releases color cache shaders/textures; unknown dynamic backends no longer alias to oakgl - SharedMemoryRegion POSIX attach validates segment size; ReadMessage skips blank lines instead of failing; GC counter clamped; IsRenderingCustomRange implemented; TimeOffsetNode gets a true inverse OutputTimeAdjustment; zero-speed clips return the held frame Plugin/nodes: - OliveClip: stored default region of definition is honored, on-demand images are cached; OliveHost sets host identity properties and logs instead of showing modal dialogs offscreen; Plugin.h dead decls gone - DespillNode guards graph-less use with Rec.709 fallback; description typos fixed (despill, swirl); mosaic applies when only one axis matches; Windows-only Project filename separator test fixed --- app/audio/audiomanager.cpp | 13 +++- app/audio/audiovisualwaveform.cpp | 27 ++++--- app/node/audio/pan/pan.cpp | 5 +- app/node/block/clip/clip.cpp | 4 +- app/node/distort/swirl/swirldistortnode.cpp | 2 +- app/node/effect/opacity/opacityeffect.cpp | 1 + app/node/filter/mosaic/mosaicfilternode.cpp | 4 +- app/node/group/group.cpp | 5 +- app/node/inputimmediate.cpp | 17 ++++- app/node/keying/despill/despill.cpp | 10 ++- app/node/math/math/math.cpp | 1 - app/node/math/math/mathbase.cpp | 9 ++- app/node/math/trigonometry/trigonometry.cpp | 2 - app/node/node.cpp | 20 ++++- app/node/nodeundo.cpp | 8 +- app/node/nodeundo.h | 3 + app/node/output/track/tracklist.cpp | 10 ++- app/node/output/track/tracklist.h | 6 ++ app/node/output/viewer/viewer.cpp | 2 +- app/node/plugins/Plugin.h | 3 - app/node/project/footage/footage.cpp | 52 +++++++++---- .../project/footage/footagedescription.cpp | 6 +- app/node/project/sequence/sequence.h | 3 + app/node/project/serializer/serializer.cpp | 6 +- app/node/time/timeoffset/timeoffsetnode.cpp | 21 ++++-- app/node/time/timeoffset/timeoffsetnode.h | 1 + app/node/value.cpp | 17 +++-- app/pluginSupport/OliveClip.cpp | 12 +++ app/pluginSupport/OliveHost.cpp | 48 +++++++++++- app/pluginSupport/OliveHost.h | 2 +- app/render/backend/dynamicrenderer.cpp | 13 +++- app/render/ipc/ipcmessage.cpp | 51 +++++++------ app/render/ipc/sharedmemoryregion.cpp | 20 +++++ app/render/previewautocacher.cpp | 12 ++- app/render/renderer.cpp | 23 ++++-- app/render/renderjobtracker.cpp | 2 +- app/render/rendermanager.cpp | 24 +++++- app/render/rendermanager.h | 6 +- tests/CMakeLists.txt | 2 +- tests/gtest/CMakeLists.txt | 2 +- tests/gtest/audio_manager_viewer_test.cpp | 7 +- tests/gtest/audio_waveform_test.cpp | 7 +- tests/gtest/footage_probe_test.cpp | 15 ++-- tests/gtest/footage_test.cpp | 23 ++++++ tests/gtest/node_audio_test.cpp | 16 ++-- tests/gtest/node_distort_test.cpp | 5 +- tests/gtest/node_filter_keying_test.cpp | 55 +++++++++++++- tests/gtest/node_group_test.cpp | 7 +- tests/gtest/node_math_test.cpp | 33 ++++----- tests/gtest/node_math_transition_test.cpp | 39 +++++----- tests/gtest/node_time_test.cpp | 24 ++++-- tests/gtest/node_undo_test.cpp | 18 +++++ tests/gtest/node_value_extended_test.cpp | 73 ++++++++++--------- tests/gtest/plugin_node_test.cpp | 72 +++++++++++++++++- tests/gtest/plugin_paraminstance_test.cpp | 38 +++++++++- tests/gtest/plugin_support_clip_test.cpp | 9 ++- tests/gtest/project_factory_test.cpp | 6 +- tests/gtest/render_ipc_test.cpp | 27 +++++++ tests/gtest/render_misc_test.cpp | 7 +- tests/gtest/render_workerpool_ipc_test.cpp | 8 +- tests/gtest/sequence_test.cpp | 21 ++++++ 61 files changed, 731 insertions(+), 254 deletions(-) diff --git a/app/audio/audiomanager.cpp b/app/audio/audiomanager.cpp index 5dc929e53..8f4bce0d4 100644 --- a/app/audio/audiomanager.cpp +++ b/app/audio/audiomanager.cpp @@ -197,6 +197,8 @@ void AudioManager::SetOutputDevice(PaDeviceIndex device) { if (device == paNoDevice) { qInfo() << "No output device found"; + } else if (device < 0 || device >= Pa_GetDeviceCount()) { + qWarning() << "Invalid output audio device index:" << device; } else { qInfo() << "Setting output audio device to" << Pa_GetDeviceInfo(device)->name; @@ -205,12 +207,16 @@ void AudioManager::SetOutputDevice(PaDeviceIndex device) output_device_ = device; CloseOutputStream(); + + emit OutputParamsChanged(); } void AudioManager::SetInputDevice(PaDeviceIndex device) { if (device == paNoDevice) { qInfo() << "No input device found"; + } else if (device < 0 || device >= Pa_GetDeviceCount()) { + qWarning() << "Invalid input audio device index:" << device; } else { qInfo() << "Setting input audio device to" << Pa_GetDeviceInfo(device)->name; @@ -407,7 +413,12 @@ PaStreamParameters AudioManager::GetPortAudioParams(const AudioParams ¶ms, p.device = device; p.hostApiSpecificStreamInfo = nullptr; p.sampleFormat = GetPortAudioSampleFormat(params.format()); - p.suggestedLatency = Pa_GetDeviceInfo(device)->defaultLowOutputLatency; + + if (device >= 0 && device < Pa_GetDeviceCount()) { + p.suggestedLatency = Pa_GetDeviceInfo(device)->defaultLowOutputLatency; + } else { + p.suggestedLatency = 0; + } return p; } diff --git a/app/audio/audiovisualwaveform.cpp b/app/audio/audiovisualwaveform.cpp index efe1070df..95a3544e7 100644 --- a/app/audio/audiovisualwaveform.cpp +++ b/app/audio/audiovisualwaveform.cpp @@ -168,8 +168,9 @@ void AudioVisualWaveform::OverwriteSums(const AudioVisualWaveform &sums, size_t our_start_index = time_to_samples(dest - virtual_start_, rate_dbl); - // Get our source sample - size_t their_start_index = time_to_samples(offset, rate_dbl); + // Get our source sample, indexing with the SOURCE's channel count + size_t their_start_index = std::floor(offset.toDouble() * rate_dbl) * + sums.channel_count(); if (their_start_index >= their_arr.size()) { continue; } @@ -260,7 +261,11 @@ void AudioVisualWaveform::TrimIn(rational length) } } - length_ = qMax(rational(0), length_ - length); + if (!negative) { + length_ = qMax(rational(0), length_ - length); + } + // Prepending grows the data before the existing start, so the absolute + // end (which length_ tracks) is unchanged } AudioVisualWaveform AudioVisualWaveform::Mid(const rational &offset) const @@ -321,14 +326,16 @@ AudioVisualWaveform::GetSummaryFromTime(const rational &start, const Sample &mipmap_data = using_mipmap->second; - // Determine if the array actually has this sample - sample_length = qMin(sample_length, mipmap_data.size() - start_sample); + // Determine if the array actually has this sample. Compare in signed + // arithmetic so a start past the end of the data doesn't underflow. + qint64 available = qint64(mipmap_data.size()) - qint64(start_sample); + if (available > 0) { + sample_length = qMin(sample_length, size_t(available)); - // Based on the above `min`, if sample length <= 0, that means start_sample >= the size of the - // array and nothing can be returned. - if (sample_length > 0) { - return ReSumSamples(&mipmap_data.data()[start_sample], sample_length, - channels_); + if (sample_length > 0) { + return ReSumSamples(&mipmap_data.data()[start_sample], + sample_length, channels_); + } } // Return null samples diff --git a/app/node/audio/pan/pan.cpp b/app/node/audio/pan/pan.cpp index f80f88711..e89d4aa63 100644 --- a/app/node/audio/pan/pan.cpp +++ b/app/node/audio/pan/pan.cpp @@ -90,8 +90,9 @@ void PanNode::Value(const NodeValueRow &value, const NodeGlobals &globals, table->Push(NodeValue(NodeValue::kSamples, samples, this)); } else { // Requires job - table->Push(NodeValue::kSamples, - SampleJob(globals.time(), kSamplesInput, value), + SampleJob job(globals.time(), kSamplesInput, value); + job.Insert(kPanningInput, value); + table->Push(NodeValue::kSamples, QVariant::fromValue(job), this); } } else { diff --git a/app/node/block/clip/clip.cpp b/app/node/block/clip/clip.cpp index be13c6692..ca3ae642f 100644 --- a/app/node/block/clip/clip.cpp +++ b/app/node/block/clip/clip.cpp @@ -250,8 +250,8 @@ rational ClipBlock::MediaToSequenceTime(const rational &media_time) const double speed_value = speed(); if (qIsNull(speed_value)) { - // I don't know what to return here yet... - sequence_time = rational::NaN; + // Speed zero holds the frame at the in point, so map to that frame + sequence_time = media_in(); } else if (!qFuzzyCompare(speed_value, 1.0)) { // Divide time sequence_time = diff --git a/app/node/distort/swirl/swirldistortnode.cpp b/app/node/distort/swirl/swirldistortnode.cpp index 652526ffd..6c5d10120 100644 --- a/app/node/distort/swirl/swirldistortnode.cpp +++ b/app/node/distort/swirl/swirldistortnode.cpp @@ -71,7 +71,7 @@ QVector SwirlDistortNode::Category() const QString SwirlDistortNode::Description() const { - return tr("Distorts an image along a sine wave."); + return tr("Distorts an image by swirling it around a center point."); } void SwirlDistortNode::Retranslate() diff --git a/app/node/effect/opacity/opacityeffect.cpp b/app/node/effect/opacity/opacityeffect.cpp index f0583587b..d722c7d3c 100644 --- a/app/node/effect/opacity/opacityeffect.cpp +++ b/app/node/effect/opacity/opacityeffect.cpp @@ -32,6 +32,7 @@ const QString OpacityEffect::kValueInput = QStringLiteral("opacity_in"); OpacityEffect::OpacityEffect() { MathNode *math = new MathNode(); + math->setParent(this); math->SetOperation(MathNode::kOpMultiply); diff --git a/app/node/filter/mosaic/mosaicfilternode.cpp b/app/node/filter/mosaic/mosaicfilternode.cpp index e48af28e7..64f325f4b 100644 --- a/app/node/filter/mosaic/mosaicfilternode.cpp +++ b/app/node/filter/mosaic/mosaicfilternode.cpp @@ -59,8 +59,8 @@ void MosaicFilterNode::Value(const NodeValueRow &value, NodeValueTable *table) const { if (TexturePtr texture = value[kTextureInput].toTexture()) { - if (texture && value[kHorizInput].toInt() != texture->width() && - value[kVertInput].toInt() != texture->height()) { + if (texture && (value[kHorizInput].toInt() != texture->width() || + value[kVertInput].toInt() != texture->height())) { ShaderJob job(value); // Mipmapping makes this look weird, so we just use bilinear for finding the color of each block diff --git a/app/node/group/group.cpp b/app/node/group/group.cpp index 7739d60e2..9a44eb393 100644 --- a/app/node/group/group.cpp +++ b/app/node/group/group.cpp @@ -262,7 +262,7 @@ QString NodeGroup::AddInputPassthrough(const NodeInput &input, id = input.input(); int i = 2; while (HasInputWithID(id)) { - id = QStringLiteral("%1_%2").arg(input.name(), QString::number(i)); + id = QStringLiteral("%1_%2").arg(input.input(), QString::number(i)); i++; } } else { @@ -334,6 +334,9 @@ QString NodeGroup::GetInputName(const QString &id) const // Call GetInputName of passed through node, which may be another group NodeInput pass = GetInputFromID(id); + if (!pass.IsValid()) { + return QString(); + } return pass.node()->GetInputName(pass.input()); } diff --git a/app/node/inputimmediate.cpp b/app/node/inputimmediate.cpp index a68071ff7..51500cf0b 100644 --- a/app/node/inputimmediate.cpp +++ b/app/node/inputimmediate.cpp @@ -23,6 +23,7 @@ #include "common/lerp.h" #include "common/tohex.h" +#include "node.h" namespace olive { @@ -289,11 +290,21 @@ void NodeInputImmediate::remove_keyframe(NodeKeyframe *key) void NodeInputImmediate::delete_all_keyframes(QObject *parent) { for (NodeKeyframeTrack &track : keyframe_tracks_) { - while (!track.isEmpty()) { + // Iterate over a copy of the track, since the keyframes may be removed + // from it as we go + const NodeKeyframeTrack copy = track; + + for (NodeKeyframe *key : copy) { + if (!dynamic_cast(key->QObject::parent())) { + // Keyframe isn't parented to a node, so reparenting/deleting it + // won't remove it from the track automatically + remove_keyframe(key); + } + if (parent) { - track.first()->setParent(parent); + key->setParent(parent); } else { - delete track.first(); + delete key; } } } diff --git a/app/node/keying/despill/despill.cpp b/app/node/keying/despill/despill.cpp index 5a74a5fa9..090ed0452 100644 --- a/app/node/keying/despill/despill.cpp +++ b/app/node/keying/despill/despill.cpp @@ -61,7 +61,7 @@ QVector DespillNode::Category() const QString DespillNode::Description() const { - return tr("Selection of simple depsill operations"); + return tr("Selection of simple despill operations"); } void DespillNode::Retranslate() @@ -95,7 +95,13 @@ void DespillNode::Value(const NodeValueRow &value, const NodeGlobals &globals, // Set luma coefficients double luma_coeffs[3] = { 0.0f, 0.0f, 0.0f }; - project()->color_manager()->GetDefaultLumaCoefs(luma_coeffs); + if (project() && project()->color_manager()) { + project()->color_manager()->GetDefaultLumaCoefs(luma_coeffs); + } else { + luma_coeffs[0] = 0.2126; + luma_coeffs[1] = 0.7152; + luma_coeffs[2] = 0.0722; + } job.Insert( QStringLiteral("luma_coeffs"), NodeValue(NodeValue::kVec3, diff --git a/app/node/math/math/math.cpp b/app/node/math/math/math.cpp index 14a2f1028..2113f40db 100644 --- a/app/node/math/math/math.cpp +++ b/app/node/math/math/math.cpp @@ -85,7 +85,6 @@ void MathNode::Retranslate() GetOperationName(kOpSubtract), GetOperationName(kOpMultiply), GetOperationName(kOpDivide), - QString(), GetOperationName(kOpPower) }; SetComboBoxStrings(kMethodIn, operations); diff --git a/app/node/math/math/mathbase.cpp b/app/node/math/math/mathbase.cpp index d29da7f8d..9bf6ed1cc 100644 --- a/app/node/math/math/mathbase.cpp +++ b/app/node/math/math/mathbase.cpp @@ -323,11 +323,14 @@ void MathNodeBase::ValueInternal( QVector4D vec = (NodeValue::type_is_vector(val_a.type()) ? RetrieveVector(val_a) : RetrieveVector(val_b)); - float number = - RetrieveNumber((val_a.type() & NodeValue::kMatrix) ? val_b : val_a); + float number = RetrieveNumber(NodeValue::type_is_vector(val_a.type()) ? + val_b : + val_a); // Only multiply and divide are valid operations - PushVector(output, val_a.type(), + PushVector(output, + NodeValue::type_is_vector(val_a.type()) ? val_a.type() : + val_b.type(), PerformMultDiv(operation, vec, number)); break; } diff --git a/app/node/math/trigonometry/trigonometry.cpp b/app/node/math/trigonometry/trigonometry.cpp index 90d5bab1e..2e7d8854e 100644 --- a/app/node/math/trigonometry/trigonometry.cpp +++ b/app/node/math/trigonometry/trigonometry.cpp @@ -64,11 +64,9 @@ void TrigonometryNode::Retranslate() QStringList strings = { tr("Sine"), tr("Cosine"), tr("Tangent"), - QString(), tr("Inverse Sine"), tr("Inverse Cosine"), tr("Inverse Tangent"), - QString(), tr("Hyperbolic Sine"), tr("Hyperbolic Cosine"), tr("Hyperbolic Tangent") }; diff --git a/app/node/node.cpp b/app/node/node.cpp index 5f5865ba5..5164ffd03 100644 --- a/app/node/node.cpp +++ b/app/node/node.cpp @@ -497,8 +497,19 @@ QVariant Node::GetSplitValueAtTimeOnTrack(const QString &input, double before_val, after_val, interpolated; if (type == NodeValue::kRational) { - before_val = before->value().value().toDouble(); - after_val = after->value().value().toDouble(); + // Keys for rational inputs usually hold rationals, but may + // hold plain doubles, in which case we convert to rational + // first to preserve the value + before_val = (before->value().canConvert() ? + before->value().value() : + rational::fromDouble( + before->value().toDouble())) + .toDouble(); + after_val = (after->value().canConvert() ? + after->value().value() : + rational::fromDouble( + after->value().toDouble())) + .toDouble(); } else { before_val = before->value().toDouble(); after_val = after->value().toDouble(); @@ -1724,7 +1735,10 @@ bool Node::LoadImmediate(QXmlStreamReader *reader, const QString &input, key->set_element(element); key->set_track(track); - key->load(reader, data_type); + if (!key->load(reader, data_type)) { + delete key; + return false; + } key->setParent(this); } else { reader->skipCurrentElement(); diff --git a/app/node/nodeundo.cpp b/app/node/nodeundo.cpp index 786cf25cf..0eef74d56 100644 --- a/app/node/nodeundo.cpp +++ b/app/node/nodeundo.cpp @@ -109,7 +109,7 @@ void NodeSetPositionAndDependenciesRecursivelyCommand::move_recursively( { Node::Position pos = context_->GetNodePositionDataInContext(node); pos += diff; - commands_.append(new NodeSetPositionCommand(node_, context_, pos)); + commands_.append(new NodeSetPositionCommand(node, context_, pos)); for (auto it = node->input_connections().cbegin(); it != node->input_connections().cend(); it++) { @@ -610,6 +610,10 @@ void NodeImmediateRemoveAllKeyframesCommand::prepare() for (const NodeKeyframeTrack &track : immediate_->keyframe_tracks()) { keys_.append(track); } + + if (!keys_.isEmpty()) { + node_ = keys_.first()->parent(); + } } void NodeImmediateRemoveAllKeyframesCommand::redo() @@ -622,7 +626,7 @@ void NodeImmediateRemoveAllKeyframesCommand::redo() void NodeImmediateRemoveAllKeyframesCommand::undo() { for (auto it = keys_.crbegin(); it != keys_.crend(); it++) { - (*it)->setParent(&memory_manager_); + (*it)->setParent(node_); } } diff --git a/app/node/nodeundo.h b/app/node/nodeundo.h index 997fdad2b..c8d408e7c 100644 --- a/app/node/nodeundo.h +++ b/app/node/nodeundo.h @@ -823,6 +823,7 @@ class NodeImmediateRemoveAllKeyframesCommand : public UndoCommand { public: NodeImmediateRemoveAllKeyframesCommand(NodeInputImmediate *immediate) : immediate_(immediate) + , node_(nullptr) { } @@ -841,6 +842,8 @@ protected: private: NodeInputImmediate *immediate_; + Node *node_; + QObject memory_manager_; QVector keys_; diff --git a/app/node/output/track/tracklist.cpp b/app/node/output/track/tracklist.cpp index 08d104e96..dac924796 100644 --- a/app/node/output/track/tracklist.cpp +++ b/app/node/output/track/tracklist.cpp @@ -85,10 +85,11 @@ void TrackList::TrackConnected(Node *node, int element) connect(track, &Track::TrackLengthChanged, this, &TrackList::UpdateTotalLength); - connect(track, &Track::TrackHeightChanged, this, [this]() { - Track *t = static_cast(sender()); - emit TrackHeightChanged(t, t->GetTrackHeightInPixels()); - }); + track_height_connections_.insert( + track, connect(track, &Track::TrackHeightChanged, this, [this]() { + Track *t = static_cast(sender()); + emit TrackHeightChanged(t, t->GetTrackHeightInPixels()); + })); track->set_type(type_); track->set_sequence(parent()); @@ -134,6 +135,7 @@ void TrackList::TrackDisconnected(Node *node, int element) disconnect(track, &Track::TrackLengthChanged, this, &TrackList::UpdateTotalLength); + disconnect(track_height_connections_.take(track)); emit TrackListChanged(); diff --git a/app/node/output/track/tracklist.h b/app/node/output/track/tracklist.h index a02657d80..b5271b30c 100644 --- a/app/node/output/track/tracklist.h +++ b/app/node/output/track/tracklist.h @@ -22,6 +22,7 @@ #ifndef TRACKLIST_H #define TRACKLIST_H +#include #include #include "node/output/track/track.h" @@ -113,6 +114,11 @@ private: QVector track_cache_; QVector track_array_indexes_; + /** + * @brief Stored TrackHeightChanged connections so they can be disconnected again + */ + QHash track_height_connections_; + QString track_input_; rational total_length_; diff --git a/app/node/output/viewer/viewer.cpp b/app/node/output/viewer/viewer.cpp index 21ef97f76..fb77dbea0 100644 --- a/app/node/output/viewer/viewer.cpp +++ b/app/node/output/viewer/viewer.cpp @@ -460,7 +460,7 @@ void ViewerOutput::Value(const NodeValueRow &value, const NodeGlobals &globals, if (HasInputWithID(kSamplesInput)) { NodeValue repush = value[kSamplesInput]; repush.set_tag(Track::Reference(Track::kAudio, 0).ToString()); - table->Push(value[kSamplesInput]); + table->Push(repush); } } diff --git a/app/node/plugins/Plugin.h b/app/node/plugins/Plugin.h index 27ec3e094..c46894939 100644 --- a/app/node/plugins/Plugin.h +++ b/app/node/plugins/Plugin.h @@ -40,9 +40,6 @@ public: QString SubCategory() const override; QString Description() const override; - void AddPushButton(); - void AddPage(); - Node *copy() const override; /** * @brief The main processing function diff --git a/app/node/project/footage/footage.cpp b/app/node/project/footage/footage.cpp index fb2e65399..ae27503f4 100644 --- a/app/node/project/footage/footage.cpp +++ b/app/node/project/footage/footage.cpp @@ -182,11 +182,20 @@ int Footage::GetStreamIndex(Track::Type type, int index) const { switch (type) { case Track::kVideo: - return GetVideoParams(index).stream_index(); + if (index >= 0 && index < GetVideoStreamCount()) { + return GetVideoParams(index).stream_index(); + } + break; case Track::kAudio: - return GetAudioParams(index).stream_index(); + if (index >= 0 && index < GetAudioStreamCount()) { + return GetAudioParams(index).stream_index(); + } + break; case Track::kSubtitle: - return GetSubtitleParams(index).stream_index(); + if (index >= 0 && index < GetSubtitleStreamCount()) { + return GetSubtitleParams(index).stream_index(); + } + break; case Track::kNone: case Track::kCount: break; @@ -476,18 +485,29 @@ rational Footage::AdjustTimeByLoopMode(rational time, LoopMode loop_mode, time = rational::NaN; break; case LoopMode::kLoopModeClamp: - // Clamp footage time to length - time = std::clamp(time, rational(0), length - timebase); + if (length < timebase) { + // No full frame fits in the range, so there is nothing to clamp to + time = rational::NaN; + } else { + // Clamp footage time to length + time = std::clamp(time, rational(0), length - timebase); + } break; case LoopMode::kLoopModeLoop: - // Loop footage time around job length - do { - if (time >= length) { - time -= length; - } else { - time += length; - } - } while (TimeIsOutOfBounds(time, length)); + if (length <= 0) { + // Cannot loop around an empty range + time = rational::NaN; + } else { + // Loop footage time around job length + do { + if (time >= length) { + time -= length; + } else { + time += length; + } + } while (TimeIsOutOfBounds(time, length)); + } + break; } } @@ -797,7 +817,10 @@ void Footage::Reprobe() } if (!cancelled_ || !cancelled_->HeardCancel()) { - if (!footage_info.Save(meta_cache_file)) { + // Only cache successful probes; caching a failed probe + // would make every future load re-use the invalid metadata + if (footage_info.IsValid() && + !footage_info.Save(meta_cache_file)) { qWarning() << "Failed to save stream cache, footage will have to be re-probed"; } @@ -898,6 +921,7 @@ void Footage::CheckFootage() if (current_file_timestamp != timestamp()) { // File has changed! + Clear(); Reprobe(); InvalidateAll(kFilenameInput); } diff --git a/app/node/project/footage/footagedescription.cpp b/app/node/project/footage/footagedescription.cpp index 81848cc63..28f88a46b 100644 --- a/app/node/project/footage/footagedescription.cpp +++ b/app/node/project/footage/footagedescription.cpp @@ -41,8 +41,11 @@ bool FootageDescription::Load(const QString &filename) if (file.open(QFile::ReadOnly)) { QXmlStreamReader reader(&file); + bool found_streamcache = false; + while (XMLReadNextStartElement(&reader)) { if (reader.name() == QStringLiteral("streamcache")) { + found_streamcache = true; // Default to first version of metadata (which wasn't versioned at all) unsigned version = 1; @@ -126,7 +129,8 @@ bool FootageDescription::Load(const QString &filename) qWarning() << "Failed to load footage description for" << filename << reader.errorString(); } else { - return true; + // Only accept files whose root element is the one Save() writes + return found_streamcache; } } diff --git a/app/node/project/sequence/sequence.h b/app/node/project/sequence/sequence.h index 94c8c36ff..4a5ac34f3 100644 --- a/app/node/project/sequence/sequence.h +++ b/app/node/project/sequence/sequence.h @@ -70,6 +70,9 @@ public: Track *GetTrackFromReference(const Track::Reference &track_ref) const { + if (track_ref.type() < 0 || track_ref.type() >= track_lists_.size()) { + return nullptr; + } return track_lists_.at(track_ref.type())->GetTrackAt(track_ref.index()); } diff --git a/app/node/project/serializer/serializer.cpp b/app/node/project/serializer/serializer.cpp index 699634068..7cd8e3d5c 100644 --- a/app/node/project/serializer/serializer.cpp +++ b/app/node/project/serializer/serializer.cpp @@ -120,9 +120,11 @@ ProjectSerializer::Result ProjectSerializer::Load(Project *project, if (attr.name() == QStringLiteral("version")) { // 230220+ projects version = attr.value().toUInt(); - } else if (reader->name() == + } else if (attr.name() == QStringLiteral("url")) { // 230220+ projects - project->SetSavedURL(attr.value().toString()); + if (project) { + project->SetSavedURL(attr.value().toString()); + } } } diff --git a/app/node/time/timeoffset/timeoffsetnode.cpp b/app/node/time/timeoffset/timeoffsetnode.cpp index f0c1f8754..6bf3c1eec 100644 --- a/app/node/time/timeoffset/timeoffsetnode.cpp +++ b/app/node/time/timeoffset/timeoffsetnode.cpp @@ -66,14 +66,14 @@ TimeRange TimeOffsetNode::OutputTimeAdjustment(const QString &input, int element, const TimeRange &input_time) const { - /*if (input == kInputInput) { - rational target_time = GetValueAtTime(kTimeInput, input_time.in()).value(); - - return TimeRange(target_time, target_time + input_time.length()); - } else { - return super::OutputTimeAdjustment(input, element, input_time); - }*/ - return super::OutputTimeAdjustment(input, element, input_time); + if (input == kInputInput) { + // The inverse of InputTimeAdjustment(): times at the input are mapped + // back to the output by subtracting the offset again + return TimeRange(GetRemappedOutputTime(input_time.in()), + GetRemappedOutputTime(input_time.out())); + } else { + return super::OutputTimeAdjustment(input, element, input_time); + } } void TimeOffsetNode::Value(const NodeValueRow &value, @@ -88,4 +88,9 @@ rational TimeOffsetNode::GetRemappedTime(const rational &input) const return input + GetValueAtTime(kTimeInput, input).value(); } +rational TimeOffsetNode::GetRemappedOutputTime(const rational &input) const +{ + return input - GetValueAtTime(kTimeInput, input).value(); +} + } diff --git a/app/node/time/timeoffset/timeoffsetnode.h b/app/node/time/timeoffset/timeoffsetnode.h index 5eba48e21..5f89534f3 100644 --- a/app/node/time/timeoffset/timeoffsetnode.h +++ b/app/node/time/timeoffset/timeoffsetnode.h @@ -70,6 +70,7 @@ public: private: rational GetRemappedTime(const rational &input) const; + rational GetRemappedOutputTime(const rational &input) const; }; } diff --git a/app/node/value.cpp b/app/node/value.cpp index aabcaa76d..aae95c56b 100644 --- a/app/node/value.cpp +++ b/app/node/value.cpp @@ -261,6 +261,8 @@ QString NodeValue::GetPrettyDataTypeName(Type type) case kInt: case kCombo: return QCoreApplication::translate("NodeValue", "Integer"); + case kStrCombo: + return QCoreApplication::translate("NodeValue", "String Combo"); case kFloat: return QCoreApplication::translate("NodeValue", "Float"); case kRational: @@ -297,6 +299,8 @@ QString NodeValue::GetPrettyDataTypeName(Type type) return QCoreApplication::translate("NodeValue", "Subtitle Parameters"); case kBinary: return QCoreApplication::translate("NodeValue", "Binary"); + case kPushButton: + return QCoreApplication::translate("NodeValue", "Push Button"); case kDataTypeCount: break; @@ -314,6 +318,8 @@ QString NodeValue::GetDataTypeName(Type type) return QStringLiteral("int"); case kCombo: return QStringLiteral("combo"); + case kStrCombo: + return QStringLiteral("strcombo"); case kFloat: return QStringLiteral("float"); case kRational: @@ -350,6 +356,8 @@ QString NodeValue::GetDataTypeName(Type type) return QStringLiteral("sparam"); case kBinary: return QStringLiteral("binary"); + case kPushButton: + return QStringLiteral("pushbutton"); case kDataTypeCount: break; } @@ -399,7 +407,7 @@ bool NodeValueTable::Has(NodeValue::Type type) const for (int i = values_.size() - 1; i >= 0; i--) { const NodeValue &v = values_.at(i); - if (v.type() & type) { + if (v.type() == type) { return true; } } @@ -463,12 +471,9 @@ int NodeValueTable::GetValueIndex(const QVector &types, for (int i = values_.size() - 1; i >= 0; i--) { const NodeValue &v = values_.at(i); - if (types.contains(v.type())) { + if (types.contains(v.type()) && (tag.isEmpty() || tag == v.tag())) { index = i; - - if (tag.isEmpty() || tag == v.tag()) { - break; - } + break; } } diff --git a/app/pluginSupport/OliveClip.cpp b/app/pluginSupport/OliveClip.cpp index 8d01d28de..78079dec7 100644 --- a/app/pluginSupport/OliveClip.cpp +++ b/app/pluginSupport/OliveClip.cpp @@ -467,7 +467,14 @@ olive::plugin::OliveClipInstance::getImage(OfxTime time, return nullptr; } + // Cache the on-demand image like the output path does, so repeated + // fetches at the same time reuse it and getConnected() reflects it. + // The extra reference keeps the cached image alive when the plugin + // releases its own. + pruneImagesCache(); Image *image = new Image(*this, preferred_params, bounds, rod, true); + images_.insert(time, image); + image->addReference(); return image; } } @@ -546,6 +553,11 @@ olive::plugin::OliveClipInstance::getRegionOfDefinition(OfxTime time) const double par = params_.pixel_aspect_ratio().toDouble(); regionOfDefinition.x2 = params_.width() * par; regionOfDefinition.y2 = params_.height(); + if (regionOfDefinition.x2 <= 0 || regionOfDefinition.y2 <= 0) { + // The params provide no usable region; fall back to the default set + // via setDefaultRegionOfDefinition(). + return defaultRegionOfDefinitions_; + } return regionOfDefinition; } void olive::plugin::OliveClipInstance::setRegionOfDefinition( diff --git a/app/pluginSupport/OliveHost.cpp b/app/pluginSupport/OliveHost.cpp index da2aac90e..812c1512f 100644 --- a/app/pluginSupport/OliveHost.cpp +++ b/app/pluginSupport/OliveHost.cpp @@ -33,6 +33,7 @@ #include "OlivePluginInstance.h" #include "common/Current.h" #include "ofxMessage.h" +#include "version.h" #include using namespace OFX::Host; using namespace olive::plugin; @@ -113,6 +114,26 @@ void olive::plugin::loadPlugins(QString path) } cache->scanPluginFiles(); } +OliveHost::OliveHost() +{ + // Identify the host to plugins; HostSupport seeds these with "UNKNOWN". + _properties.setStringProperty(kOfxPropName, "Oak Video Editor"); + _properties.setStringProperty(kOfxPropLabel, "Oak Video Editor"); + _properties.setStringProperty(kOfxPropVersionLabel, + olive::kAppVersion.toStdString()); + + // Numeric version for plugins that query kOfxPropVersion directly. + const QStringList version_parts = + olive::kAppVersion.section(QLatin1Char('-'), 0, 0) + .split(QLatin1Char('.')); + _properties.setIntProperty(kOfxPropVersion, + version_parts.value(0).toInt(), 0); + _properties.setIntProperty(kOfxPropVersion, + version_parts.value(1).toInt(), 1); + _properties.setIntProperty(kOfxPropVersion, + version_parts.value(2).toInt(), 2); +} + OliveHost::~OliveHost() { } @@ -184,7 +205,9 @@ OfxStatus olive::plugin::OliveHost::vmessage(const char *type, const char *id, QString message(buffer); auto *app = qobject_cast(QCoreApplication::instance()); - if (!app) { + // A modal dialog would hang a headless (offscreen) session, so log to + // stderr instead of showing one. + if (!app || QGuiApplication::platformName() == QLatin1String("offscreen")) { qWarning().noquote() << "OFX message:" << type << message; if (strcmp(type, kOfxMessageQuestion) == 0) { return kOfxStatReplyNo; @@ -223,15 +246,32 @@ OfxStatus olive::plugin::OliveHost::setPersistentMessage(const char *type, vsnprintf(buffer, sizeof(buffer), format, args); QString message(buffer); + // A modal dialog would hang a headless (offscreen) session, so log to + // stderr instead of showing one. + const bool headless = + QGuiApplication::platformName() == QLatin1String("offscreen"); + if (strcmp(type, kOfxMessageError) == 0) { persistent_messages_.append({ HostMessageType::Error, message }); - QMessageBox::critical(nullptr, "", message); + if (headless) { + qWarning().noquote() << "OFX error:" << message; + } else { + QMessageBox::critical(nullptr, "", message); + } } else if (strcmp(type, kOfxMessageWarning) == 0) { persistent_messages_.append({ HostMessageType::Warning, message }); - QMessageBox::warning(nullptr, "", message); + if (headless) { + qWarning().noquote() << "OFX warning:" << message; + } else { + QMessageBox::warning(nullptr, "", message); + } } else if (strcmp(type, kOfxMessageMessage) == 0) { persistent_messages_.append({ HostMessageType::Message, message }); - QMessageBox::information(nullptr, "", message); + if (headless) { + qWarning().noquote() << "OFX message:" << message; + } else { + QMessageBox::information(nullptr, "", message); + } } else { return kOfxStatFailed; } diff --git a/app/pluginSupport/OliveHost.h b/app/pluginSupport/OliveHost.h index cc5b4ea44..8ac2cc0b7 100644 --- a/app/pluginSupport/OliveHost.h +++ b/app/pluginSupport/OliveHost.h @@ -44,7 +44,7 @@ struct HostPersistentMessage { void loadPlugins(QString path); class OliveHost : public OFX::Host::ImageEffect::Host { public: - OliveHost() = default; + OliveHost(); ~OliveHost() override; void destroyInstance(OFX::Host::ImageEffect::Instance *instance); diff --git a/app/render/backend/dynamicrenderer.cpp b/app/render/backend/dynamicrenderer.cpp index 9683a1bb0..ee72751c9 100644 --- a/app/render/backend/dynamicrenderer.cpp +++ b/app/render/backend/dynamicrenderer.cpp @@ -37,9 +37,16 @@ DynamicRenderer::~DynamicRenderer() // system libGL/libvulkan loader is never mistaken for an Oak render backend. QString DynamicRenderer::LibraryFilename() const { - const QString base = backend_ == QStringLiteral("vulkan") ? - QStringLiteral("oakvulkan") : - QStringLiteral("oakgl"); + QString base; + if (backend_ == QStringLiteral("opengl")) { + base = QStringLiteral("oakgl"); + } else if (backend_ == QStringLiteral("vulkan")) { + base = QStringLiteral("oakvulkan"); + } else { + // Unknown backend: use the name verbatim so the load fails and the + // caller's OpenGL fallback engages + base = backend_; + } #if defined(Q_OS_WIN) const QString filename = base + QStringLiteral(".dll"); #elif defined(Q_OS_MAC) diff --git a/app/render/ipc/ipcmessage.cpp b/app/render/ipc/ipcmessage.cpp index a5ec9a984..90aac2a09 100644 --- a/app/render/ipc/ipcmessage.cpp +++ b/app/render/ipc/ipcmessage.cpp @@ -38,37 +38,36 @@ bool WriteMessage(QIODevice *device, const QJsonObject &obj) bool ReadMessage(QByteArray *buffer, QJsonObject *out, bool *ok) { - const int newline = buffer->indexOf('\n'); - if (newline < 0) { - // No complete line buffered yet. - return false; - } - - const QByteArray line = buffer->left(newline); - buffer->remove(0, newline + 1); - - // Skip blank lines silently (e.g. a stray newline) without flagging an error. - if (line.trimmed().isEmpty()) { - if (ok) { - *ok = false; + while (true) { + const int newline = buffer->indexOf('\n'); + if (newline < 0) { + // No complete line buffered yet. + return false; } - return false; - } - QJsonParseError err; - const QJsonDocument doc = QJsonDocument::fromJson(line, &err); - if (err.error != QJsonParseError::NoError || !doc.isObject()) { - if (ok) { - *ok = false; + const QByteArray line = buffer->left(newline); + buffer->remove(0, newline + 1); + + // Skip blank lines silently (e.g. a stray newline) without flagging an error. + if (line.trimmed().isEmpty()) { + continue; } - return false; - } - *out = doc.object(); - if (ok) { - *ok = true; + QJsonParseError err; + const QJsonDocument doc = QJsonDocument::fromJson(line, &err); + if (err.error != QJsonParseError::NoError || !doc.isObject()) { + if (ok) { + *ok = false; + } + return false; + } + + *out = doc.object(); + if (ok) { + *ok = true; + } + return true; } - return true; } // ---- HandshakeMsg --------------------------------------------------------------------------- diff --git a/app/render/ipc/sharedmemoryregion.cpp b/app/render/ipc/sharedmemoryregion.cpp index c671f5a0c..3d58382d9 100644 --- a/app/render/ipc/sharedmemoryregion.cpp +++ b/app/render/ipc/sharedmemoryregion.cpp @@ -166,6 +166,26 @@ bool SharedMemoryRegion::Open(const QString &key, size_t size, Mode mode) shm_unlink(name_bytes.constData()); return false; } + } else { + // mmap() succeeds even beyond the real segment size and only faults + // (SIGBUS) on access, so verify the segment is large enough up front. + struct stat st; + if (fstat(fd_, &st) != 0) { + error_ = QStringLiteral("fstat failed: %1") + .arg(QString::fromUtf8(strerror(errno))); + ::close(fd_); + fd_ = -1; + return false; + } + if (st.st_size < off_t(size)) { + error_ = QStringLiteral( + "shared memory segment is %1 bytes, smaller than the requested %2") + .arg(qint64(st.st_size)) + .arg(qint64(size)); + ::close(fd_); + fd_ = -1; + return false; + } } data_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd_, 0); diff --git a/app/render/previewautocacher.cpp b/app/render/previewautocacher.cpp index 193528109..8af52f35b 100644 --- a/app/render/previewautocacher.cpp +++ b/app/render/previewautocacher.cpp @@ -504,8 +504,16 @@ void PreviewAutoCacher::CancelAudioTasks(bool and_wait_for_them_to_finish) bool PreviewAutoCacher::IsRenderingCustomRange() const { - /*const VideoCacheData &d = video_cache_data_.value(viewer_node_); - return d.iterator.IsCustomRange() && d.iterator.HasNext();*/ + if (!use_custom_range_) { + return false; + } + + for (const VideoJob &job : pending_video_jobs_) { + if (job.range == custom_autocache_range_ && job.iterator.HasNext()) { + return true; + } + } + return false; } diff --git a/app/render/renderer.cpp b/app/render/renderer.cpp index 839535767..b471143eb 100644 --- a/app/render/renderer.cpp +++ b/app/render/renderer.cpp @@ -129,16 +129,24 @@ QVariant Renderer::GetDefaultShader() void Renderer::Destroy() { - destroyed_ = true; - if (lifetime_) { - lifetime_->alive = false; - } if (!default_shader_.isNull()) { DestroyNativeShader(default_shader_); default_shader_.clear(); } - color_cache_.clear(); + { + QMutexLocker locker(&color_cache_mutex_); + + // Destroy the cached native shaders explicitly. The LUT textures are + // TexturePtrs whose destructors call DestroyTexture(), so the cache must + // be cleared while the renderer is still alive for those to be honored. + for (auto it = color_cache_.begin(); it != color_cache_.end(); it++) { + if (!it->compiled_shader.isNull()) { + DestroyNativeShader(it->compiled_shader); + } + } + color_cache_.clear(); + } if (!interlace_texture_.isNull()) { DestroyNativeShader(interlace_texture_); @@ -150,6 +158,11 @@ void Renderer::Destroy() } texture_cache_.clear(); + destroyed_ = true; + if (lifetime_) { + lifetime_->alive = false; + } + DestroyInternal(); } diff --git a/app/render/renderjobtracker.cpp b/app/render/renderjobtracker.cpp index 36c762cb3..db3991d37 100644 --- a/app/render/renderjobtracker.cpp +++ b/app/render/renderjobtracker.cpp @@ -26,7 +26,7 @@ namespace olive void RenderJobTracker::insert(const TimeRange &range, JobTime job_time) { - // First remove any ranges with this (code copied + // First remove any ranges that overlap this one (code copied from TimeRangeList::remove) TimeRangeList::util_remove(&jobs_, range); // Now append the job diff --git a/app/render/rendermanager.cpp b/app/render/rendermanager.cpp index 5bd808695..168bff95c 100644 --- a/app/render/rendermanager.cpp +++ b/app/render/rendermanager.cpp @@ -220,7 +220,12 @@ RenderTicketPtr RenderManager::RenderFrame(const RenderVideoParams ¶ms) } if (worker_params.return_type == ReturnType::kNull) { - dry_run_thread_->AddTicket(ticket); + if (dry_run_thread_) { + dry_run_thread_->AddTicket(ticket); + } else { + // No render threads (e.g. dummy backend), finish without a result + ticket->Finish(); + } } else if (worker_pool_ && worker_pool_->SubmitFrame(ticket, worker_params)) { return ticket; @@ -247,13 +252,16 @@ RenderTicketPtr RenderManager::RenderAudio(const RenderAudioParams ¶ms) ticket->setProperty("aparam", QVariant::fromValue(params.audio_params)); ticket->setProperty("mode", params.mode); - if (params.generate_waveforms) { + if (params.generate_waveforms && !waveform_threads_.empty()) { size_t thread_index = last_waveform_thread_ % waveform_threads_.size(); RenderThread *thread = waveform_threads_[thread_index]; thread->AddTicket(ticket); last_waveform_thread_++; - } else { + } else if (audio_thread_) { audio_thread_->AddTicket(ticket); + } else { + // No render threads (e.g. dummy backend), finish without a result + ticket->Finish(); } return ticket; @@ -278,6 +286,11 @@ void RenderManager::SetAggressiveGarbageCollection(bool enabled) { aggressive_gc_ += enabled ? 1 : -1; + // Clamp at zero so unbalanced disable calls can't drive the counter negative + if (aggressive_gc_ < 0) { + aggressive_gc_ = 0; + } + if (aggressive_gc_ > 0) { decoder_clear_timer_->setInterval(kDecoderMaximumInactivityAggressive); } else { @@ -287,6 +300,11 @@ void RenderManager::SetAggressiveGarbageCollection(bool enabled) void RenderManager::ClearOldDecoders() { + if (!decoder_cache_) { + // No decoder cache exists on backends without a renderer (e.g. dummy) + return; + } + QMutexLocker locker(decoder_cache_->mutex()); qint64 min_age = diff --git a/app/render/rendermanager.h b/app/render/rendermanager.h index dbf96be1c..325ba295a 100644 --- a/app/render/rendermanager.h +++ b/app/render/rendermanager.h @@ -255,11 +255,11 @@ private: QTimer *decoder_clear_timer_; - RenderThread *dry_run_thread_; - RenderThread *audio_thread_; + RenderThread *dry_run_thread_ = nullptr; + RenderThread *audio_thread_ = nullptr; std::vector waveform_threads_; - size_t last_waveform_thread_; + size_t last_waveform_thread_ = 0; std::list render_threads_; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7fa46d0a8..54d063d30 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -40,7 +40,7 @@ function(olive_add_test GROUP NAME SOURCE) string(APPEND TEST_FILE_CONTENT "\n${TEST_BODY}") file(WRITE "${OUTPUT_FILE}" "${TEST_FILE_CONTENT}") - add_executable(${NAME} ${OUTPUT_FILE} $) + add_executable(${NAME} ${OUTPUT_FILE} $ $) target_include_directories( ${NAME} PRIVATE diff --git a/tests/gtest/CMakeLists.txt b/tests/gtest/CMakeLists.txt index 7381205e7..68596ec23 100644 --- a/tests/gtest/CMakeLists.txt +++ b/tests/gtest/CMakeLists.txt @@ -116,7 +116,7 @@ add_executable(olive-gtest find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Test) -target_sources(olive-gtest PRIVATE $) +target_sources(olive-gtest PRIVATE $ $) target_include_directories( olive-gtest diff --git a/tests/gtest/audio_manager_viewer_test.cpp b/tests/gtest/audio_manager_viewer_test.cpp index d378c8137..0e040d0a1 100644 --- a/tests/gtest/audio_manager_viewer_test.cpp +++ b/tests/gtest/audio_manager_viewer_test.cpp @@ -583,9 +583,12 @@ TEST_F(ViewerOutputTest, ValueRepushTagsStreams) EXPECT_EQ(table.Get(olive::NodeValue::kTexture, video_tag).type(), olive::NodeValue::kTexture); - // The samples value is re-pushed and stays retrievable - EXPECT_EQ(table.Get(olive::NodeValue::kSamples).type(), + // The samples value is re-pushed tagged as audio stream 0 + const QString audio_tag = + olive::Track::Reference(olive::Track::kAudio, 0).ToString(); + EXPECT_EQ(table.Get(olive::NodeValue::kSamples, audio_tag).type(), olive::NodeValue::kSamples); + EXPECT_EQ(table.Get(olive::NodeValue::kSamples).tag(), audio_tag); } TEST_F(ViewerOutputTest, LastUsedEncodingParamsRoundTrip) diff --git a/tests/gtest/audio_waveform_test.cpp b/tests/gtest/audio_waveform_test.cpp index fd7847e53..2d143565e 100644 --- a/tests/gtest/audio_waveform_test.cpp +++ b/tests/gtest/audio_waveform_test.cpp @@ -225,10 +225,9 @@ TEST(AudioVisualWaveform, OverwriteSamplesBeforeExistingDataPrependsZeros) ASSERT_EQ(summary.size(), 1); ExpectSummary(summary, 0, 0.75f, 0.75f); - // BUG: the data now spans [0, 3), but prepending via a negative TrimIn - // subtracts the negated length from length_ instead of keeping the - // absolute end time, so length() reports 1 instead of 3 - EXPECT_EQ(waveform.length(), olive::core::rational(1)); + // The data now spans [0, 3): prepending via a negative TrimIn keeps the + // absolute end time tracked by length() + EXPECT_EQ(waveform.length(), olive::core::rational(3)); } // --------------------------------------------------------------------------- diff --git a/tests/gtest/footage_probe_test.cpp b/tests/gtest/footage_probe_test.cpp index fbbd64e0b..5aa4c1d8b 100644 --- a/tests/gtest/footage_probe_test.cpp +++ b/tests/gtest/footage_probe_test.cpp @@ -439,9 +439,8 @@ TEST_F(FootageProbeTest, CheckFootageOnlyRespondsWithActiveWindow) EXPECT_TRUE(footage->IsValid()); // With an active window, CheckFootage notices the missing file and - // re-probes. The re-probe resets the timestamp but, because Reprobe() - // never clears existing state for a missing file, the (now stale) probe - // data is kept until the filename itself changes. + // re-probes. The re-probe clears the existing state first, and since the + // file no longer exists, the footage is left invalid with no streams. { QWidget window; window.show(); @@ -455,8 +454,8 @@ TEST_F(FootageProbeTest, CheckFootageOnlyRespondsWithActiveWindow) ASSERT_EQ(qApp->activeWindow(), nullptr); EXPECT_EQ(footage->timestamp(), 0); - EXPECT_TRUE(footage->IsValid()); - EXPECT_EQ(footage->GetVideoStreamCount(), 1); + EXPECT_FALSE(footage->IsValid()); + EXPECT_EQ(footage->GetVideoStreamCount(), 0); } TEST_F(FootageProbeTest, ProbingExistingButInvalidMediaStaysInvalid) @@ -480,7 +479,7 @@ TEST_F(FootageProbeTest, ProbingExistingButInvalidMediaStaysInvalid) EXPECT_EQ(footage->GetAudioStreamCount(), 0); EXPECT_EQ(footage->GetSubtitleStreamCount(), 0); - // Note: Reprobe caches even this failed probe result, so future reprobes - // of the same path reload the invalid description instead of re-probing - EXPECT_TRUE(QFileInfo::exists(MetadataCacheFileFor(path))); + // A failed probe is not written to the metadata cache, so future reprobes + // of the same path probe again instead of reloading an invalid description + EXPECT_FALSE(QFileInfo::exists(MetadataCacheFileFor(path))); } diff --git a/tests/gtest/footage_test.cpp b/tests/gtest/footage_test.cpp index a4b054544..29b555129 100644 --- a/tests/gtest/footage_test.cpp +++ b/tests/gtest/footage_test.cpp @@ -301,6 +301,23 @@ TEST(FootageStatic, AdjustTimeByLoopModeLoopsAroundLength) olive::rational(5)); } +TEST(FootageStatic, AdjustTimeByLoopModeWithEmptyRangeReturnsNaN) +{ + // Looping an empty range would never terminate; return NaN instead + EXPECT_TRUE(olive::Footage::AdjustTimeByLoopMode( + olive::rational(1), olive::LoopMode::kLoopModeLoop, + olive::rational(0), olive::VideoParams::kVideoTypeVideo, + olive::rational(1, 24)) + .isNaN()); + + // Clamping a range shorter than one frame has no frame to clamp to + EXPECT_TRUE(olive::Footage::AdjustTimeByLoopMode( + olive::rational(1), olive::LoopMode::kLoopModeClamp, + olive::rational(0), olive::VideoParams::kVideoTypeVideo, + olive::rational(1, 24)) + .isNaN()); +} + TEST(FootageStatic, RetranslateSetsInputNames) { TestableFootage footage; @@ -375,6 +392,12 @@ TEST_F(FootageTest, ManuallyAddedStreamsMapBetweenReferencesAndIndices) EXPECT_EQ(footage.GetStreamIndex(olive::Track::kNone, 0), -1); EXPECT_EQ(footage.GetStreamIndex(olive::Track::kCount, 0), -1); + // Out-of-range indices report -1 rather than a default-constructed stream + EXPECT_EQ(footage.GetStreamIndex(olive::Track::kVideo, 1), -1); + EXPECT_EQ(footage.GetStreamIndex(olive::Track::kVideo, -1), -1); + EXPECT_EQ(footage.GetStreamIndex(olive::Track::kAudio, 1), -1); + EXPECT_EQ(footage.GetStreamIndex(olive::Track::kSubtitle, 1), -1); + EXPECT_EQ(footage.GetReferenceFromRealIndex(5), olive::Track::Reference(olive::Track::kVideo, 0)); EXPECT_EQ(footage.GetReferenceFromRealIndex(2), diff --git a/tests/gtest/node_audio_test.cpp b/tests/gtest/node_audio_test.cpp index 8021b1aae..287a73618 100644 --- a/tests/gtest/node_audio_test.cpp +++ b/tests/gtest/node_audio_test.cpp @@ -364,7 +364,7 @@ TEST(PanNode, NoSamplesInputProducesNoOutput) olive::NodeValue::kNone); } -TEST(PanNode, KeyframedPanProducesSampleJobButLosesPanValue) +TEST(PanNode, KeyframedPanProducesSampleJobWithPanValue) { olive::ColorManager::SetUpDefaultConfig(); olive::Project project; @@ -383,22 +383,22 @@ TEST(PanNode, KeyframedPanProducesSampleJobButLosesPanValue) ASSERT_EQ(result.type(), olive::NodeValue::kSamples); ASSERT_TRUE(result.canConvert()); - // NOTE: Unlike VolumeNode, PanNode::Value() never inserts the panning - // value into the SampleJob, so the job's value map is empty and - // ProcessSamples() always sees a pan of 0 (a plain copy). The production - // RenderProcessor only re-evaluates inputs present in the job, so - // keyframed pan is silently ignored (suspected bug, documented here). + // Like VolumeNode, PanNode::Value() inserts the panning value into the + // SampleJob so ProcessSamples() sees the keyframed pan const olive::SampleJob job = result.value(); - EXPECT_FALSE(job.GetValues().contains(olive::PanNode::kPanningInput)); + ASSERT_TRUE(job.GetValues().contains(olive::PanNode::kPanningInput)); + EXPECT_DOUBLE_EQ(job.GetValues().value(olive::PanNode::kPanningInput).toDouble(), + 1.0); SampleResolvingTraverser resolver; resolver.Resolve(result); + // Pan 1.0 (full right) silences the left channel and leaves the right const olive::core::SampleBuffer out = result.toSamples(); ASSERT_TRUE(out.is_allocated()); ASSERT_EQ(out.sample_count(), 4u); for (int i = 0; i < 4; i++) { - EXPECT_FLOAT_EQ(out.data(0)[i], float(i + 1)); + EXPECT_FLOAT_EQ(out.data(0)[i], 0.0f); EXPECT_FLOAT_EQ(out.data(1)[i], float(i + 5)); } } diff --git a/tests/gtest/node_distort_test.cpp b/tests/gtest/node_distort_test.cpp index 2d3553d1f..5a6395c3e 100644 --- a/tests/gtest/node_distort_test.cpp +++ b/tests/gtest/node_distort_test.cpp @@ -1362,9 +1362,8 @@ TEST(SwirlDistortNode, MetadataIsCorrect) // NOTE: "org.oliveeditor.*" domain, inconsistent with most Olive nodes EXPECT_EQ(node.id(), QStringLiteral("org.oliveeditor.Olive.swirl")); EXPECT_EQ(node.Name(), QStringLiteral("Swirl")); - // NOTE: the description reads "Distorts an image along a sine wave.", - // identical to WaveDistortNode's (copy-paste, documented here) - EXPECT_FALSE(node.Description().isEmpty()); + EXPECT_EQ(node.Description(), + QStringLiteral("Distorts an image by swirling it around a center point.")); EXPECT_TRUE(node.Category().contains(olive::Node::kCategoryDistort)); EXPECT_TRUE(node.GetFlags() & olive::Node::kVideoEffect); diff --git a/tests/gtest/node_filter_keying_test.cpp b/tests/gtest/node_filter_keying_test.cpp index bb711d70d..8a3842841 100644 --- a/tests/gtest/node_filter_keying_test.cpp +++ b/tests/gtest/node_filter_keying_test.cpp @@ -836,6 +836,28 @@ TEST(MosaicFilterNode, ValueWithMatchingResolutionPassesTextureThrough) EXPECT_FALSE(out->IsJob()); } +TEST(MosaicFilterNode, ValueWithSingleAxisMatchingResolutionRunsJob) +{ + olive::MosaicFilterNode node; + + // Only one axis matching the texture size still changes the image, so + // the effect must run; passthrough requires BOTH axes to match. + olive::TexturePtr tex = MakeDummyTexture(); + olive::NodeValueRow row = + MakeTextureRow(olive::MosaicFilterNode::kTextureInput, tex); + row.insert(olive::MosaicFilterNode::kHorizInput, FloatValue(16.0)); + row.insert(olive::MosaicFilterNode::kVertInput, FloatValue(8.0)); + + olive::NodeValueTable table; + node.Value(row, olive::NodeGlobals(), &table); + + ASSERT_EQ(table.Count(), 1); + const olive::TexturePtr out = + table.Get(olive::NodeValue::kTexture).toTexture(); + ASSERT_TRUE(out); + EXPECT_TRUE(out->IsJob()); +} + TEST(MosaicFilterNode, ValuePushesJobWithLinearInterpolation) { olive::MosaicFilterNode node; @@ -1444,8 +1466,6 @@ TEST(DespillNode, ShaderCodeLoadsFragmentResource) TEST(DespillNode, ValueInProjectWithoutTexturePushesNothing) { - // DespillNode::Value() unconditionally queries the project's color - // manager, so it can only be exercised with the node in a project. olive::ColorManager::SetUpDefaultConfig(); olive::Project project; @@ -1501,3 +1521,34 @@ TEST(DespillNode, ValueInProjectPushesJobWithLumaCoefficients) EXPECT_EQ(values.value(olive::DespillNode::kTextureInput).toTexture(), tex); } + +TEST(DespillNode, ValueWithoutProjectUsesRec709LumaFallback) +{ + // A graph-less node has no project color manager; it must fall back to + // Rec. 709 luma coefficients instead of crashing. + olive::DespillNode node; + + olive::TexturePtr tex = MakeDummyTexture(); + olive::NodeValueRow row = + MakeTextureRow(olive::DespillNode::kTextureInput, tex); + + olive::NodeValueTable table; + node.Value(row, olive::NodeGlobals(), &table); + + ASSERT_EQ(table.Count(), 1); + const olive::TexturePtr out = + table.Get(olive::NodeValue::kTexture).toTexture(); + ASSERT_TRUE(out); + ASSERT_TRUE(out->IsJob()); + + auto *job = static_cast(out->job()); + ASSERT_NE(job, nullptr); + + const olive::NodeValueRow &values = job->GetValues(); + ASSERT_TRUE(values.contains(QStringLiteral("luma_coeffs"))); + const QVector3D coeffs = + values.value(QStringLiteral("luma_coeffs")).toVec3(); + EXPECT_NEAR(coeffs.x(), 0.2126f, 0.0001f); + EXPECT_NEAR(coeffs.y(), 0.7152f, 0.0001f); + EXPECT_NEAR(coeffs.z(), 0.0722f, 0.0001f); +} diff --git a/tests/gtest/node_group_test.cpp b/tests/gtest/node_group_test.cpp index 3efa232c9..6d6a6e11b 100644 --- a/tests/gtest/node_group_test.cpp +++ b/tests/gtest/node_group_test.cpp @@ -129,15 +129,12 @@ TEST_F(NodeGroupTest, AddInputPassthroughGeneratesUniqueIdForDuplicateInputId) EXPECT_EQ(id_a, olive::MathNode::kParamAIn); // A second passthrough of the same input ID (on a different node) must - // not collide with the first - math_b->Retranslate(); + // not collide with the first; the suffix is derived from the input ID const QString id_b = group->AddInputPassthrough( olive::NodeInput(math_b, olive::MathNode::kParamAIn)); - // NOTE: the suffix is derived from the input's display name rather than - // its ID, so a retranslated MathNode param becomes "Value_2" EXPECT_NE(id_a, id_b); - EXPECT_EQ(id_b, QStringLiteral("Value_2")); + EXPECT_EQ(id_b, QStringLiteral("param_a_in_2")); ASSERT_EQ(group->GetInputPassthroughs().size(), 2); EXPECT_TRUE(group->HasInputWithID(id_b)); diff --git a/tests/gtest/node_math_test.cpp b/tests/gtest/node_math_test.cpp index ee38ee218..8b32c7feb 100644 --- a/tests/gtest/node_math_test.cpp +++ b/tests/gtest/node_math_test.cpp @@ -214,16 +214,14 @@ TEST(MathNode, RetranslateSetsInputNamesAndComboStrings) math.GetInputProperty(olive::MathNode::kMethodIn, QStringLiteral("combo_str")) .toStringList(); - ASSERT_EQ(operations.size(), 6); + ASSERT_EQ(operations.size(), 5); EXPECT_EQ(operations.at(0), QStringLiteral("Add")); EXPECT_EQ(operations.at(1), QStringLiteral("Subtract")); EXPECT_EQ(operations.at(2), QStringLiteral("Multiply")); EXPECT_EQ(operations.at(3), QStringLiteral("Divide")); - // NOTE: kOpPower == 4, but the combo list has an empty string at index 4 - // and "Power" at index 5, so the combo box is misaligned with the - // Operation enum (suspected bug, documented here). - EXPECT_TRUE(operations.at(4).isEmpty()); - EXPECT_EQ(operations.at(5), QStringLiteral("Power")); + // The combo list matches the Operation enum exactly, so kOpPower == 4 + // selects "Power" + EXPECT_EQ(operations.at(4), QStringLiteral("Power")); } TEST(MathNode, AddNumbers) @@ -720,7 +718,7 @@ TEST(MathNode, AddVectorAndNumberIsNoOp) EXPECT_FLOAT_EQ(result.toVec2().y(), 2.0f); } -TEST(MathNode, NumberTimesVectorYieldsNoComputedValue) +TEST(MathNode, NumberTimesVectorScalesVector) { olive::ColorManager::SetUpDefaultConfig(); olive::Project project; @@ -736,18 +734,17 @@ TEST(MathNode, NumberTimesVectorYieldsNoComputedValue) QVector4D(1.0f, 2.0f, 3.0f, 4.0f))); olive::Node::ConnectEdge(b, olive::NodeInput(math, olive::MathNode::kParamBIn)); - // NOTE: With the number in parameter A and the vector in parameter B, - // ValueInternal() picks the *vector* as the number operand (the - // `val_a.type() & NodeValue::kMatrix` check is true for kFloat) and then - // pushes the result with type kFloat, which PushVector() drops. No - // computed value is produced, and nothing passes through into the output - // table either (suspected bug, documented here). + // With the number in parameter A and the vector in parameter B, the + // number is still picked as the number operand and the vector is scaled, + // mirroring MultiplyVectorByNumber with the operands swapped olive::NodeValueTable table = GenerateMathTable(math); - - EXPECT_EQ(table.Get(olive::NodeValue::kVec4).type(), - olive::NodeValue::kNone); - EXPECT_EQ(table.Get(olive::NodeValue::kFloat).type(), - olive::NodeValue::kNone); + olive::NodeValue result = table.Get(olive::NodeValue::kVec4); + ASSERT_EQ(result.type(), olive::NodeValue::kVec4); + const QVector4D vec = result.toVec4(); + EXPECT_FLOAT_EQ(vec.x(), 2.0f); + EXPECT_FLOAT_EQ(vec.y(), 4.0f); + EXPECT_FLOAT_EQ(vec.z(), 6.0f); + EXPECT_FLOAT_EQ(vec.w(), 8.0f); } TEST(MathNode, MultiplyMatrixByVector) diff --git a/tests/gtest/node_math_transition_test.cpp b/tests/gtest/node_math_transition_test.cpp index e41290c91..61dd87655 100644 --- a/tests/gtest/node_math_transition_test.cpp +++ b/tests/gtest/node_math_transition_test.cpp @@ -184,23 +184,19 @@ TEST(TrigonometryNode, RetranslateSetsInputNamesAndComboStrings) node.GetInputProperty(olive::TrigonometryNode::kMethodIn, QStringLiteral("combo_str")) .toStringList(); - ASSERT_EQ(methods.size(), 11); + ASSERT_EQ(methods.size(), 9); EXPECT_EQ(methods.at(0), QStringLiteral("Sine")); EXPECT_EQ(methods.at(1), QStringLiteral("Cosine")); EXPECT_EQ(methods.at(2), QStringLiteral("Tangent")); - EXPECT_TRUE(methods.at(3).isEmpty()); - EXPECT_EQ(methods.at(4), QStringLiteral("Inverse Sine")); - EXPECT_EQ(methods.at(5), QStringLiteral("Inverse Cosine")); - EXPECT_EQ(methods.at(6), QStringLiteral("Inverse Tangent")); - EXPECT_TRUE(methods.at(7).isEmpty()); - EXPECT_EQ(methods.at(8), QStringLiteral("Hyperbolic Sine")); - EXPECT_EQ(methods.at(9), QStringLiteral("Hyperbolic Cosine")); - EXPECT_EQ(methods.at(10), QStringLiteral("Hyperbolic Tangent")); + EXPECT_EQ(methods.at(3), QStringLiteral("Inverse Sine")); + EXPECT_EQ(methods.at(4), QStringLiteral("Inverse Cosine")); + EXPECT_EQ(methods.at(5), QStringLiteral("Inverse Tangent")); + EXPECT_EQ(methods.at(6), QStringLiteral("Hyperbolic Sine")); + EXPECT_EQ(methods.at(7), QStringLiteral("Hyperbolic Cosine")); + EXPECT_EQ(methods.at(8), QStringLiteral("Hyperbolic Tangent")); - // NOTE: The combo list contains separator entries at indexes 3 and 7 that - // the Operation enum used by Value() does not have, so combo indexes 4 - // and above no longer match the enum (suspected bug, documented here and - // in ComboIndexBeyondSeparatorComputesWrongFunction). + // The combo list contains no separator entries, so combo indexes match + // the Operation enum used by Value() exactly } TEST(TrigonometryNode, SineCosineTangent) @@ -264,7 +260,7 @@ TEST(TrigonometryNode, HyperbolicOperations) EXPECT_NEAR(GenerateTrigResult(node), std::tanh(1.0), 1e-12); } -TEST(TrigonometryNode, ComboIndexBeyondSeparatorComputesWrongFunction) +TEST(TrigonometryNode, ComboIndexMatchesOperationEnum) { olive::ColorManager::SetUpDefaultConfig(); olive::Project project; @@ -272,15 +268,20 @@ TEST(TrigonometryNode, ComboIndexBeyondSeparatorComputesWrongFunction) auto *node = AddNode(&project); - // NOTE: The combo box labels index 4 as "Inverse Sine", but Value() - // casts the index to the Operation enum where 4 is kOpArcCosine, because - // the combo string list contains separator entries that the enum does - // not have (suspected bug, test documents current behavior). + // The combo list has no separator entries, so a combo index selects the + // Operation enum value with the same index + node->SetStandardValue(olive::TrigonometryNode::kMethodIn, 3); + node->SetStandardValue(olive::TrigonometryNode::kXIn, 0.5); + EXPECT_NEAR(GenerateTrigResult(node), std::asin(0.5), 1e-12); + node->SetStandardValue(olive::TrigonometryNode::kMethodIn, 4); node->SetStandardValue(olive::TrigonometryNode::kXIn, 0.5); EXPECT_NEAR(GenerateTrigResult(node), std::acos(0.5), 1e-12); - // Index 8 is labeled "Hyperbolic Sine" but computes hyperbolic tangent + node->SetStandardValue(olive::TrigonometryNode::kMethodIn, 6); + node->SetStandardValue(olive::TrigonometryNode::kXIn, 1.0); + EXPECT_NEAR(GenerateTrigResult(node), std::sinh(1.0), 1e-12); + node->SetStandardValue(olive::TrigonometryNode::kMethodIn, 8); node->SetStandardValue(olive::TrigonometryNode::kXIn, 1.0); EXPECT_NEAR(GenerateTrigResult(node), std::tanh(1.0), 1e-12); diff --git a/tests/gtest/node_time_test.cpp b/tests/gtest/node_time_test.cpp index 361ea985c..b7f996d22 100644 --- a/tests/gtest/node_time_test.cpp +++ b/tests/gtest/node_time_test.cpp @@ -184,22 +184,34 @@ TEST_F(NodeTimeTest, TimeOffsetZeroOffsetIsIdentity) range); } -TEST_F(NodeTimeTest, TimeOffsetOutputAdjustmentPassesThrough) +TEST_F(NodeTimeTest, TimeOffsetOutputAdjustmentAppliesInverseOffset) { auto *offset = AddNode(); offset->SetStandardValue(olive::TimeOffsetNode::kTimeInput, QVariant::fromValue(olive::core::rational(3))); - // The inverse mapping is not implemented, so output time is never - // adjusted + // The inverse mapping subtracts the offset again: input-side times are + // mapped back to the output by the negated offset + EXPECT_EQ(offset->OutputTimeAdjustment( + olive::TimeOffsetNode::kInputInput, -1, + olive::TimeRange(olive::core::rational(2), + olive::core::rational(4))), + olive::TimeRange(olive::core::rational(-1), + olive::core::rational(1))); + + // Non-input inputs never adjust time const olive::TimeRange range(olive::core::rational(2), olive::core::rational(4)); - EXPECT_EQ(offset->OutputTimeAdjustment(olive::TimeOffsetNode::kInputInput, - -1, range), - range); EXPECT_EQ(offset->OutputTimeAdjustment(olive::TimeOffsetNode::kTimeInput, -1, range), range); + + // Round trip through both adjustments returns the original range + EXPECT_EQ(offset->OutputTimeAdjustment( + olive::TimeOffsetNode::kInputInput, -1, + offset->InputTimeAdjustment( + olive::TimeOffsetNode::kInputInput, -1, range, true)), + range); } TEST_F(NodeTimeTest, TimeOffsetAppliesKeyframedOffset) diff --git a/tests/gtest/node_undo_test.cpp b/tests/gtest/node_undo_test.cpp index 66e6c815a..48acfe41a 100644 --- a/tests/gtest/node_undo_test.cpp +++ b/tests/gtest/node_undo_test.cpp @@ -243,8 +243,15 @@ TEST_F(NodeUndoTest, SetPositionCommandRestoresPreviousPosition) TEST_F(NodeUndoTest, SetPositionAndDependenciesRecursivelyMovesNode) { + auto *dep = AddNode(); auto *node = AddNode(); auto *context = AddNode(); + + olive::Node::ConnectEdge( + dep, olive::NodeInput(node, olive::MathNode::kParamAIn)); + + context->SetNodePositionInContext( + dep, olive::Node::Position(QPointF(1.0, 1.0))); context->SetNodePositionInContext( node, olive::Node::Position(QPointF(2.0, 3.0))); @@ -253,9 +260,12 @@ TEST_F(NodeUndoTest, SetPositionAndDependenciesRecursivelyMovesNode) cmd.redo_now(); EXPECT_EQ(context->GetNodePositionInContext(node), QPointF(8.0, 9.0)); + // The dependency moves by the same delta as the node + EXPECT_EQ(context->GetNodePositionInContext(dep), QPointF(7.0, 7.0)); cmd.undo_now(); EXPECT_EQ(context->GetNodePositionInContext(node), QPointF(2.0, 3.0)); + EXPECT_EQ(context->GetNodePositionInContext(dep), QPointF(1.0, 1.0)); } TEST_F(NodeUndoTest, RemovePositionFromContextCommandRestoresPosition) @@ -909,4 +919,12 @@ TEST_F(NodeUndoTest, ImmediateRemoveAllKeyframesCommandRemovesKeys) EXPECT_TRUE(immediate->keyframe_tracks().at(0).isEmpty()); EXPECT_NE(key_a->parent(), node); EXPECT_NE(key_b->parent(), node); + + cmd.undo_now(); + // Undo restores the keyframes to the node and its tracks + ASSERT_EQ(immediate->keyframe_tracks().at(0).size(), 2); + EXPECT_EQ(immediate->keyframe_tracks().at(0).at(0), key_a); + EXPECT_EQ(immediate->keyframe_tracks().at(0).at(1), key_b); + EXPECT_EQ(key_a->parent(), node); + EXPECT_EQ(key_b->parent(), node); } diff --git a/tests/gtest/node_value_extended_test.cpp b/tests/gtest/node_value_extended_test.cpp index 67b064de0..ad4c5fa48 100644 --- a/tests/gtest/node_value_extended_test.cpp +++ b/tests/gtest/node_value_extended_test.cpp @@ -416,14 +416,14 @@ TEST(NodeValueExtended, PrettyDataTypeNames) olive::NodeValue::kDataTypeCount), QStringLiteral("Unknown")); - // NOTE: kStrCombo and kPushButton have no dedicated pretty name and fall - // through to "Unknown" (naming gap, documented here). + // kStrCombo and kPushButton have dedicated pretty names like every other + // type EXPECT_EQ( olive::NodeValue::GetPrettyDataTypeName(olive::NodeValue::kStrCombo), - QStringLiteral("Unknown")); + QStringLiteral("String Combo")); EXPECT_EQ( olive::NodeValue::GetPrettyDataTypeName(olive::NodeValue::kPushButton), - QStringLiteral("Unknown")); + QStringLiteral("Push Button")); } TEST(NodeValueExtended, TypeClassificationRemainingCases) @@ -463,14 +463,12 @@ TEST(NodeValueExtended, TypeClassificationRemainingCases) EXPECT_FALSE(olive::NodeValue::type_is_buffer(olive::NodeValue::kFloat)); } -TEST(NodeValueExtended, UnnamedTypesHaveEmptyDataTypeNames) +TEST(NodeValueExtended, AllRealTypesHaveDataTypeNames) { - EXPECT_TRUE( - olive::NodeValue::GetDataTypeName(olive::NodeValue::kStrCombo) - .isEmpty()); - EXPECT_TRUE( - olive::NodeValue::GetDataTypeName(olive::NodeValue::kPushButton) - .isEmpty()); + EXPECT_EQ(olive::NodeValue::GetDataTypeName(olive::NodeValue::kStrCombo), + QStringLiteral("strcombo")); + EXPECT_EQ(olive::NodeValue::GetDataTypeName(olive::NodeValue::kPushButton), + QStringLiteral("pushbutton")); EXPECT_TRUE( olive::NodeValue::GetDataTypeName(olive::NodeValue::kDataTypeCount) .isEmpty()); @@ -479,11 +477,17 @@ TEST(NodeValueExtended, UnnamedTypesHaveEmptyDataTypeNames) QStringLiteral("not-a-type")), olive::NodeValue::kNone); - // NOTE: an empty name matches the first type with an empty serialized - // name (kStrCombo) rather than producing kNone (suspected bug, documented - // here). + // An empty name matches no type EXPECT_EQ(olive::NodeValue::GetDataTypeFromName(QString()), - olive::NodeValue::kStrCombo); + olive::NodeValue::kNone); + + // The newly named types round-trip + EXPECT_EQ( + olive::NodeValue::GetDataTypeFromName(QStringLiteral("strcombo")), + olive::NodeValue::kStrCombo); + EXPECT_EQ( + olive::NodeValue::GetDataTypeFromName(QStringLiteral("pushbutton")), + olive::NodeValue::kPushButton); } TEST(NodeValueExtended, ArrayValuesRoundTrip) @@ -536,13 +540,14 @@ TEST(NodeValueTableExtended, GetWithTagSelectsMatchingValue) QStringLiteral("b")), 1); - // NOTE: an unknown tag does not yield an empty value; the search keeps - // scanning and returns the oldest value of the type instead (suspected - // bug, documented here). - EXPECT_DOUBLE_EQ( - table.Get(olive::NodeValue::kFloat, QStringLiteral("missing")) - .toDouble(), - 1.0); + // An unknown tag yields an empty value; the fallback to the oldest value + // of the type only applies when no tag is requested + olive::NodeValue missing = + table.Get(olive::NodeValue::kFloat, QStringLiteral("missing")); + EXPECT_EQ(missing.type(), olive::NodeValue::kNone); + EXPECT_EQ(table.GetValueIndex({ olive::NodeValue::kFloat }, + QStringLiteral("missing")), + -1); } TEST(NodeValueTableExtended, GetWithMultipleTypes) @@ -603,12 +608,13 @@ TEST(NodeValueTableExtended, TakeWithTagAndMissingType) EXPECT_EQ(absent.type(), olive::NodeValue::kNone); EXPECT_EQ(table.Count(), 1); - // NOTE: like Get(), an unmatched tag falls back to the oldest value of - // the type (suspected bug, documented here). + // Taking with an unmatched tag returns an empty value and leaves the table + // unchanged; the oldest value of the type is not used as a fallback olive::NodeValue fallback = table.Take(olive::NodeValue::kText, QStringLiteral("missing")); - EXPECT_EQ(fallback.toString(), QStringLiteral("b")); - EXPECT_TRUE(table.isEmpty()); + EXPECT_EQ(fallback.type(), olive::NodeValue::kNone); + ASSERT_EQ(table.Count(), 1); + EXPECT_EQ(table.at(0).toString(), QStringLiteral("b")); } TEST(NodeValueTableExtended, TakeWithMultipleTypes) @@ -657,24 +663,21 @@ TEST(NodeValueTableExtended, RemoveDeletesNewestEqualValue) EXPECT_EQ(table.Count(), 2); } -TEST(NodeValueTableExtended, HasUsesBitmaskComparison) +TEST(NodeValueTableExtended, HasUsesExactTypeMatch) { olive::NodeValueTable table; table.Push(olive::NodeValue(olive::NodeValue::kFloat, 1.0)); EXPECT_TRUE(table.Has(olive::NodeValue::kFloat)); EXPECT_FALSE(table.Has(olive::NodeValue::kInt)); - // NOTE: Has() compares types with a bitwise AND even though Type is a - // sequential enum, so unrelated types alias: kFloat (2) also satisfies - // kRational (3) and kText (7) because 2 & 3 != 0 and 2 & 7 != 0 - // (suspected bug, documented here). - EXPECT_TRUE(table.Has(olive::NodeValue::kRational)); - EXPECT_TRUE(table.Has(olive::NodeValue::kText)); + // Type is a sequential enum, so no other type aliases kFloat + EXPECT_FALSE(table.Has(olive::NodeValue::kRational)); + EXPECT_FALSE(table.Has(olive::NodeValue::kText)); - // kNone is zero, so a table holding a kNone value never reports it + // A table holding a kNone value reports it too olive::NodeValueTable none_table; none_table.Push(olive::NodeValue()); - EXPECT_FALSE(none_table.Has(olive::NodeValue::kNone)); + EXPECT_TRUE(none_table.Has(olive::NodeValue::kNone)); } TEST(NodeValueTableExtended, PushTableAppendsAllValues) diff --git a/tests/gtest/plugin_node_test.cpp b/tests/gtest/plugin_node_test.cpp index 9250e4af8..89f78d27a 100644 --- a/tests/gtest/plugin_node_test.cpp +++ b/tests/gtest/plugin_node_test.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include "ofxCore.h" @@ -34,6 +35,7 @@ #include "common/Current.h" #include "node/plugins/Plugin.h" #include "pluginSupport/OliveHost.h" +#include "version.h" namespace { @@ -202,11 +204,12 @@ TEST(OliveHost, DestroyInstanceIgnoresNull) } // ============================================================================ -// OliveHost message routing (error paths only) +// OliveHost message routing // -// The successful vmessage/setPersistentMessage paths pop modal QMessageBox -// dialogs whenever a QApplication exists, which a headless test cannot -// dismiss, so only the early-return failure paths are exercised here. +// With a QApplication on a non-offscreen platform the successful +// vmessage/setPersistentMessage paths pop modal QMessageBox dialogs, which a +// headless test cannot dismiss; on the offscreen platform they log to stderr +// instead, so those paths are exercised here behind a platform check. // ============================================================================ TEST(OliveHost, VMessageRejectsNullArguments) @@ -236,6 +239,44 @@ TEST(OliveHost, SetPersistentMessageRejectsUnknownType) kOfxStatFailed); } +TEST(OliveHost, VMessageOffscreenLogsInsteadOfDialog) +{ + if (QGuiApplication::platformName() != QLatin1String("offscreen")) { + GTEST_SKIP() << "requires the offscreen QPA platform"; + } + + olive::plugin::OliveHost host; + // No modal dialog is shown on the offscreen platform; the message is + // logged to stderr and acknowledged. + EXPECT_EQ(CallVMessage(host, kOfxMessageError, "id", "%s", "boom"), + kOfxStatOK); + EXPECT_EQ(CallVMessage(host, kOfxMessageWarning, "id", "%s", "boom"), + kOfxStatOK); + EXPECT_EQ(CallVMessage(host, kOfxMessageMessage, "id", "%s", "boom"), + kOfxStatOK); + // A question cannot be answered headlessly, so it is a "no". + EXPECT_EQ(CallVMessage(host, kOfxMessageQuestion, "id", "%s", "boom"), + kOfxStatReplyNo); +} + +TEST(OliveHost, SetPersistentMessageOffscreenSucceeds) +{ + if (QGuiApplication::platformName() != QLatin1String("offscreen")) { + GTEST_SKIP() << "requires the offscreen QPA platform"; + } + + olive::plugin::OliveHost host; + EXPECT_EQ(CallSetPersistentMessage(host, kOfxMessageError, "id", "%s", + "boom"), + kOfxStatOK); + EXPECT_EQ(CallSetPersistentMessage(host, kOfxMessageWarning, "id", "%s", + "boom"), + kOfxStatOK); + EXPECT_EQ(CallSetPersistentMessage(host, kOfxMessageMessage, "id", "%s", + "boom"), + kOfxStatOK); +} + TEST(OliveHost, ClearPersistentMessageSucceeds) { olive::plugin::OliveHost host; @@ -268,6 +309,29 @@ TEST(OliveHost, HostPropertiesIdentifyAsOfxHost) EXPECT_EQ(host.getProperties().getStringProperty(kOfxPropType), "Host"); } +TEST(OliveHost, HostPropertiesIdentifyApplication) +{ + // The ctor stamps the app identity over HostSupport's "UNKNOWN" defaults + // so plugins querying the host description see real values. + olive::plugin::OliveHost host; + const auto &props = host.getProperties(); + + EXPECT_EQ(props.getStringProperty(kOfxPropName), "Oak Video Editor"); + EXPECT_EQ(props.getStringProperty(kOfxPropLabel), "Oak Video Editor"); + EXPECT_EQ(props.getStringProperty(kOfxPropVersionLabel), + olive::kAppVersion.toStdString()); + + const QStringList version_parts = + olive::kAppVersion.section(QLatin1Char('-'), 0, 0) + .split(QLatin1Char('.')); + EXPECT_EQ(props.getIntProperty(kOfxPropVersion, 0), + version_parts.value(0).toInt()); + EXPECT_EQ(props.getIntProperty(kOfxPropVersion, 1), + version_parts.value(1).toInt()); + EXPECT_EQ(props.getIntProperty(kOfxPropVersion, 2), + version_parts.value(2).toInt()); +} + #ifdef OFX_SUPPORTS_OPENGLRENDER TEST(OliveHost, FlushOpenGLResourcesReportsFailure) { diff --git a/tests/gtest/plugin_paraminstance_test.cpp b/tests/gtest/plugin_paraminstance_test.cpp index 903e387d3..a847cb4e4 100644 --- a/tests/gtest/plugin_paraminstance_test.cpp +++ b/tests/gtest/plugin_paraminstance_test.cpp @@ -566,6 +566,38 @@ TEST(PluginClipInstance, RegionOfDefinitionPerTimeOverride) EXPECT_DOUBLE_EQ(at_four.y2, 80.0); } +TEST(PluginClipInstance, RegionOfDefinitionFallsBackToStoredDefault) +{ + OFX::Host::ImageEffect::ClipDescriptor desc(kOfxImageEffectOutputClipName); + + // Zero-sized params yield no usable params-derived region, so the stored + // default is used. + olive::VideoParams empty_params = + MakeParams(0, 0, olive::core::PixelFormat::U8, 4, false); + olive::plugin::OliveClipInstance empty_clip(nullptr, desc, empty_params); + + OfxRectD stored = { 2.0, 4.0, 202.0, 104.0 }; + empty_clip.setDefaultRegionOfDefinition(stored); + + OfxRectD fallback = empty_clip.getRegionOfDefinition(0.0); + EXPECT_DOUBLE_EQ(fallback.x1, 2.0); + EXPECT_DOUBLE_EQ(fallback.y1, 4.0); + EXPECT_DOUBLE_EQ(fallback.x2, 202.0); + EXPECT_DOUBLE_EQ(fallback.y2, 104.0); + + // A params-derived region still takes precedence over the stored default. + olive::VideoParams params = + MakeParams(100, 80, olive::core::PixelFormat::U8, 4, false); + olive::plugin::OliveClipInstance clip(nullptr, desc, params); + clip.setDefaultRegionOfDefinition(stored); + + OfxRectD derived = clip.getRegionOfDefinition(0.0); + EXPECT_DOUBLE_EQ(derived.x1, 0.0); + EXPECT_DOUBLE_EQ(derived.y1, 0.0); + EXPECT_DOUBLE_EQ(derived.x2, 100.0); + EXPECT_DOUBLE_EQ(derived.y2, 80.0); +} + TEST(PluginClipInstance, OutputImageBoundsFollowRegionOfDefinition) { OFX::Host::ImageEffect::ClipDescriptor desc(kOfxImageEffectOutputClipName); @@ -787,11 +819,11 @@ TEST(PluginClipInstance, PruneImagesCacheEvictsOldestInputImages) } clip.pruneImagesCache(); - // The oldest entry (time 1) was evicted and is recreated on demand, - // while later entries remain cached. + // The oldest entry (time 1) was evicted; its on-demand recreation is + // cached again, while later entries remained cached throughout. OFX::Host::ImageEffect::Image *evicted_a = clip.getImage(1.0, nullptr); OFX::Host::ImageEffect::Image *evicted_b = clip.getImage(1.0, nullptr); - EXPECT_NE(evicted_a, evicted_b); + EXPECT_EQ(evicted_a, evicted_b); OFX::Host::ImageEffect::Image *cached_a = clip.getImage(2.0, nullptr); OFX::Host::ImageEffect::Image *cached_b = clip.getImage(2.0, nullptr); diff --git a/tests/gtest/plugin_support_clip_test.cpp b/tests/gtest/plugin_support_clip_test.cpp index 9ba7a34fd..8e077235a 100644 --- a/tests/gtest/plugin_support_clip_test.cpp +++ b/tests/gtest/plugin_support_clip_test.cpp @@ -75,19 +75,24 @@ TEST(PluginSupportClip, GetImageClampsBoundsAndCachesOutput) EXPECT_EQ(image, image_again); } -TEST(PluginSupportClip, GetImageReturnsNewImageForNonOutput) +TEST(PluginSupportClip, GetImageCachesImageForNonOutput) { OFX::Host::ImageEffect::ClipDescriptor desc("Source"); olive::VideoParams params = MakeParams(64, 64, olive::core::PixelFormat::U8, 4, false); olive::plugin::OliveClipInstance clip(nullptr, desc, params); + EXPECT_FALSE(clip.getConnected()); + OFX::Host::ImageEffect::Image *first = clip.getImage(0.0, nullptr); OFX::Host::ImageEffect::Image *second = clip.getImage(0.0, nullptr); EXPECT_NE(first, nullptr); EXPECT_NE(second, nullptr); - EXPECT_NE(first, second); + // On-demand images are cached per time, so both fetches return the + // same image and the clip now reports as connected. + EXPECT_EQ(first, second); + EXPECT_TRUE(clip.getConnected()); first->releaseReference(); second->releaseReference(); diff --git a/tests/gtest/project_factory_test.cpp b/tests/gtest/project_factory_test.cpp index fe42c350f..cf22f4d27 100644 --- a/tests/gtest/project_factory_test.cpp +++ b/tests/gtest/project_factory_test.cpp @@ -73,10 +73,12 @@ TEST(Project, FilenameNamePrettyAndSignals) [&name_changes]() { ++name_changes; }); const QString filename = QStringLiteral("/tmp/some/dir/my_edit.ove"); + // Project::set_filename converts to native separators on Windows + const QString stored_filename = QDir::toNativeSeparators(filename); project.set_filename(filename); - EXPECT_EQ(project.filename(), filename); + EXPECT_EQ(project.filename(), stored_filename); EXPECT_EQ(project.name(), QStringLiteral("my_edit")); - EXPECT_EQ(project.pretty_filename(), filename); + EXPECT_EQ(project.pretty_filename(), stored_filename); EXPECT_FALSE(project.is_new()); EXPECT_EQ(name_changes, 1); diff --git a/tests/gtest/render_ipc_test.cpp b/tests/gtest/render_ipc_test.cpp index 55d18c9cd..42df49f27 100644 --- a/tests/gtest/render_ipc_test.cpp +++ b/tests/gtest/render_ipc_test.cpp @@ -384,6 +384,33 @@ TEST(IpcMessage, MalformedLineIsSkipped) EXPECT_TRUE(reader.isEmpty()); } +TEST(IpcMessage, BlankLinesAreSkippedSilently) +{ + CancelMsg c; + c.ticket_id = 7; + const QByteArray line = + QByteArray(QJsonDocument(c.ToJson()).toJson(QJsonDocument::Compact)) + + '\n'; + + // Blank lines (even repeated) are consumed without flagging an error, and + // the following real message is still parsed. + QByteArray reader = QByteArray("\n \n\n") + line; + QJsonObject obj; + bool ok = false; + ASSERT_TRUE(ReadMessage(&reader, &obj, &ok)); + EXPECT_TRUE(ok); + + CancelMsg c2; + ASSERT_TRUE(CancelMsg::FromJson(obj, &c2)); + EXPECT_EQ(c2.ticket_id, 7); + + // Only blank lines left: nothing more to read, but still not an error. + ok = true; + EXPECT_FALSE(ReadMessage(&reader, &obj, &ok)); + EXPECT_TRUE(ok); + EXPECT_TRUE(reader.isEmpty()); +} + TEST(IpcMessage, WrongTypeRejected) { // FromJson must reject an object whose "type" does not match the target struct. diff --git a/tests/gtest/render_misc_test.cpp b/tests/gtest/render_misc_test.cpp index 8cc4d7fa7..d16cff1db 100644 --- a/tests/gtest/render_misc_test.cpp +++ b/tests/gtest/render_misc_test.cpp @@ -216,9 +216,10 @@ TEST(DynamicRenderer, ConstructorNormalizesBackendName) EXPECT_FALSE(vk.IsOpenGL()); } -// Documents that an unrecognized backend name is kept verbatim (even though -// LibraryFilename() silently maps it to the OpenGL library basename), so the -// IsOpenGL()/IsVulkan() predicates both report false for it. +// Documents that an unrecognized backend name is kept verbatim (and +// LibraryFilename() uses that verbatim name as the library basename, so Load() +// fails and the OpenGL fallback engages), so the IsOpenGL()/IsVulkan() +// predicates both report false for it. TEST(DynamicRenderer, UnknownBackendNameIsReportedVerbatim) { olive::DynamicRenderer renderer(QStringLiteral("Metal")); diff --git a/tests/gtest/render_workerpool_ipc_test.cpp b/tests/gtest/render_workerpool_ipc_test.cpp index ca351068f..e4236ee95 100644 --- a/tests/gtest/render_workerpool_ipc_test.cpp +++ b/tests/gtest/render_workerpool_ipc_test.cpp @@ -646,16 +646,12 @@ TEST(IpcMessage, ReadMessageSkipsBlankLines) const QByteArray line = QJsonDocument(cancel.ToJson()).toJson(QJsonDocument::Compact); - // A reader loop sees: blank line, whitespace-only line, then a real message. + // A reader loop sees: blank line, whitespace-only line, then a real + // message. Blank lines are skipped silently. QByteArray reader = QByteArray("\n \n") + line + '\n'; QJsonObject obj; bool ok = true; - EXPECT_FALSE(olive::ipc::ReadMessage(&reader, &obj, &ok)); // blank - EXPECT_FALSE(ok); - EXPECT_FALSE(olive::ipc::ReadMessage(&reader, &obj, &ok)); // whitespace - EXPECT_FALSE(ok); - ASSERT_TRUE(olive::ipc::ReadMessage(&reader, &obj, &ok)); EXPECT_TRUE(ok); olive::ipc::CancelMsg back; diff --git a/tests/gtest/sequence_test.cpp b/tests/gtest/sequence_test.cpp index 5d334a106..1ebb2b216 100644 --- a/tests/gtest/sequence_test.cpp +++ b/tests/gtest/sequence_test.cpp @@ -97,6 +97,14 @@ TEST(Sequence, DefaultState) olive::Track::Reference(olive::Track::kVideo, 0)), nullptr); + // Invalid and out-of-range reference types return null instead of crashing + EXPECT_EQ(sequence.GetTrackFromReference( + olive::Track::Reference(olive::Track::kNone, 0)), + nullptr); + EXPECT_EQ(sequence.GetTrackFromReference( + olive::Track::Reference(olive::Track::kCount, 0)), + nullptr); + // Length verification over empty track lists keeps everything at zero sequence.VerifyLength(); EXPECT_EQ(sequence.GetLength(), olive::core::rational(0)); @@ -282,6 +290,15 @@ TEST(Sequence, TrackDisconnectResetsTrackState) ++sequence_removed; }); + // While connected, track height changes are forwarded by the list + int height_changed = 0; + QObject::connect(list, &olive::TrackList::TrackHeightChanged, + [&height_changed](olive::Track *, int) { + ++height_changed; + }); + first->SetTrackHeight(first->GetTrackHeight() + 1.0); + EXPECT_EQ(height_changed, 1); + olive::Node::DisconnectEdge(first, list->track_input(0)); EXPECT_EQ(list_removed, 1); @@ -311,6 +328,10 @@ TEST(Sequence, TrackDisconnectResetsTrackState) EXPECT_EQ(sequence->GetTrackFromReference( olive::Track::Reference(olive::Track::kVideo, 1)), nullptr); + + // Height changes on the removed track must no longer be forwarded + first->SetTrackHeight(first->GetTrackHeight() + 1.0); + EXPECT_EQ(height_changed, 1); } TEST(TrackList, CacheOrderFollowsArrayIndex)