Fix terrible SoundAsset::equals() implementation.

It would check individual bytes of samples to see if they differed
by more than the threshold.  Not only is this almost useless, but
the default threshold is 256 so with the default settings it would
always say that two assets of the same length (and channels, etc.)
were the same, even if the sample data was different.

diff --git a/src/sound_asset.cc b/src/sound_asset.cc
index 6752d9b1ac180a43bca43c52a93da2fc2f929505..056471be1159cb515cd870922b64ff81e5e906bf 100644 (file)
--- a/src/sound_asset.cc
+++ b/src/sound_asset.cc
@@ -201,11 +201,13 @@ SoundAsset::equals (shared_ptr<const Asset> other, EqualityOptions opt, NoteHand
                }
 
                if (memcmp (frame_A->data(), frame_B->data(), frame_A->size()) != 0) {
-                       for (int i = 0; i < frame_A->size(); ++i) {
-                               int const d = abs (frame_A->data()[i] - frame_B->data()[i]);
-                               if (d > opt.max_audio_sample_error) {
-                                       note (DCP_ERROR, String::compose ("PCM data difference of %1", d));
-                                       return false;
+                       for (int sample= 0; sample < frame_A->samples(); ++sample) {
+                               for (int channel = 0; channel < frame_A->channels(); ++channel) {
+                                       int32_t const d = abs(frame_A->get(channel, sample) - frame_B->get(channel, sample));
+                                       if (d > opt.max_audio_sample_error) {
+                                               note (DCP_ERROR, String::compose ("PCM data difference of %1", d));
+                                               return false;
+                                       }
                                }
                        }
                }

diff --git a/test/dcp_test.cc b/test/dcp_test.cc
index 167d9154c81c27a40e39b5001b88ddc75b39454a..135887d5b60a8bdfe3c892cd061c204caa2b2b5f 100644 (file)
--- a/test/dcp_test.cc
+++ b/test/dcp_test.cc
@@ -37,6 +37,7 @@
 #include "mono_picture_asset.h"
 #include "stereo_picture_asset.h"
 #include "picture_asset_writer.h"
+#include "reel_picture_asset.h"
 #include "sound_asset_writer.h"
 #include "sound_asset.h"
 #include "atmos_asset.h"
@@ -53,6 +54,7 @@
 
 using std::string;
 using std::vector;
+using boost::dynamic_pointer_cast;
 using boost::shared_ptr;
 #if BOOST_VERSION >= 106100
 using namespace boost::placeholders;
@@ -170,6 +172,89 @@ BOOST_AUTO_TEST_CASE (dcp_test4)
        BOOST_CHECK (!A.equals (B, dcp::EqualityOptions(), boost::bind (&note, _1, _2)));
 }
 
+static
+void
+test_rewriting_sound(string name, bool modify)
+{
+       dcp::DCP A ("test/ref/DCP/dcp_test1");
+       A.read ();
+
+       BOOST_REQUIRE (!A.cpls().empty());
+       BOOST_REQUIRE (!A.cpls().front()->reels().empty());
+       shared_ptr<dcp::ReelMonoPictureAsset> A_picture = dynamic_pointer_cast<dcp::ReelMonoPictureAsset>(A.cpls().front()->reels().front()->main_picture());
+       BOOST_REQUIRE (A_picture);
+       shared_ptr<dcp::ReelSoundAsset> A_sound = dynamic_pointer_cast<dcp::ReelSoundAsset>(A.cpls().front()->reels().front()->main_sound());
+
+       boost::filesystem::remove_all ("build/test/" + name);
+       dcp::DCP B ("build/test/" + name);
+       shared_ptr<dcp::Reel> reel(new dcp::Reel());
+
+       BOOST_REQUIRE (A_picture->mono_asset());
+       BOOST_REQUIRE (A_picture->mono_asset()->file());
+       boost::filesystem::copy_file (A_picture->mono_asset()->file().get(), "build/test/" +name + "/picture.mxf");
+       reel->add(
+               shared_ptr<dcp::ReelMonoPictureAsset>(
+                       new dcp::ReelMonoPictureAsset(shared_ptr<dcp::MonoPictureAsset>(new dcp::MonoPictureAsset("build/test/" + name + "/picture.mxf")), 0)
+                       )
+               );
+
+       shared_ptr<dcp::SoundAssetReader> reader = A_sound->asset()->start_read();
+       shared_ptr<dcp::SoundAsset> sound(new dcp::SoundAsset(A_sound->asset()->edit_rate(), A_sound->asset()->sampling_rate(), A_sound->asset()->channels(), dcp::LanguageTag("en-US"), dcp::SMPTE));
+       shared_ptr<dcp::SoundAssetWriter> writer = sound->start_write("build/test/" + name + "/sound.mxf", vector<dcp::Channel>());
+
+       bool need_to_modify = modify;
+       for (int i = 0; i < A_sound->asset()->intrinsic_duration(); ++i) {
+               shared_ptr<const dcp::SoundFrame> sf = reader->get_frame (i);
+               float* out[sf->channels()];
+               for (int j = 0; j < sf->channels(); ++j) {
+                       out[j] = new float[sf->samples()];
+               }
+               for (int j = 0; j < sf->samples(); ++j) {
+                       for (int k = 0; k < sf->channels(); ++k) {
+                               out[k][j] = static_cast<float>(sf->get(k, j)) / (1 << 23);
+                               if (need_to_modify) {
+                                       out[k][j] += 1.0 / (1 << 23);
+                                       need_to_modify = false;
+                               }
+                       }
+               }
+               writer->write (out, sf->samples());
+               for (int j = 0; j < sf->channels(); ++j) {
+                       delete[] out[j];
+               }
+       }
+       writer->finalize();
+
+       reel->add(shared_ptr<dcp::ReelSoundAsset>(new dcp::ReelSoundAsset(sound, 0)));
+
+       shared_ptr<dcp::CPL> cpl(new dcp::CPL("A Test DCP", dcp::FEATURE));
+       cpl->add (reel);
+
+       B.add (cpl);
+       B.write_xml (dcp::SMPTE);
+
+       dcp::EqualityOptions eq;
+       eq.reel_hashes_can_differ = true;
+       eq.max_audio_sample_error = 0;
+       if (modify) {
+               BOOST_CHECK (!A.equals(B, eq, boost::bind(&note, _1, _2)));
+       } else {
+               BOOST_CHECK (A.equals(B, eq, boost::bind(&note, _1, _2)));
+       }
+}
+
+/** Test comparison of a DCP with another that has the same picture and the same (but re-written) sound */
+BOOST_AUTO_TEST_CASE (dcp_test9)
+{
+       test_rewriting_sound ("dcp_test9", false);
+}
+
+/** Test comparison of a DCP with another that has the same picture and very slightly modified sound */
+BOOST_AUTO_TEST_CASE (dcp_test10)
+{
+       test_rewriting_sound ("dcp_test10", true);
+}
+
 /** Test creation of a 2D DCP with an Atmos track */
 BOOST_AUTO_TEST_CASE (dcp_test5)
 {