Exception safety in: subclasses of ImportFileHandle

src/import/ImportFFmpeg.cpp

@@ -847,6 +847,8 @@ void FFmpegImportFileHandle::GetMetadata(Tags *tags, const wxChar *tag, const ch
 
 FFmpegImportFileHandle::~FFmpegImportFileHandle()
 {
+   av_log_set_callback(av_log_default_callback);
+
    // Do this before unloading the libraries
    mContext.reset();
 

src/import/ImportFLAC.cpp

@@ -561,7 +561,8 @@ ProgressResult FLACImportFileHandle::Import(TrackFactory *trackFactory,
 
 FLACImportFileHandle::~FLACImportFileHandle()
 {
-   //don't DELETE mFile if we are using OD.
+   //don't finish *mFile if we are using OD,
+   //because it was not initialized in Init().
 #ifndef EXPERIMENTAL_OD_FLAC
    mFile->finish();
 #endif

src/import/ImportGStreamer.cpp

@@ -157,7 +157,8 @@ struct GStreamContext
 };
 
 // For RAII on gst objects
-template<typename T> using GstObjHandle = std::unique_ptr < T, Deleter<void, gst_object_unref > > ;
+template<typename T> using GstObjHandle =
+   std::unique_ptr < T, Deleter<void, gst_object_unref > > ;
 
 ///! Does actual import, returned by GStreamerImportPlugin::Open
 class GStreamerImportFileHandle final : public ImportFileHandle
@@ -273,17 +274,21 @@ GetGStreamerImportPlugin(ImportPluginList &importPluginList,
 
    // Initialize gstreamer
    GErrorHandle error;
+   bool initError;
+   {
       int argc = 0;
       char **argv = NULL;
       GError *ee;
-   if (!gst_init_check(&argc, &argv, &ee))
+      initError = !gst_init_check(&argc, &argv, &ee);
+      error.reset(ee);
+   }
+   if ( initError )
    {
       wxLogMessage(wxT("Failed to initialize GStreamer. Error %d: %s"),
                    error.get()->code,
                    wxString::FromUTF8(error.get()->message).c_str());
       return;
    }
-   error.reset(ee);
 
    guint major, minor, micro, nano;
    gst_version(&major, &minor, &micro, &nano);

src/import/ImportLOF.cpp

@@ -142,24 +142,23 @@ public:
 private:
    // Takes a line of text in lof file and interprets it and opens files
    void lofOpenFiles(wxString* ln);
-   void doDuration();
+   void doDurationAndScrollOffset();
-   void doScrollOffset();
 
    std::unique_ptr<wxTextFile> mTextFile;
    wxFileName mLOFFileName;  /**< The name of the LOF file, which is used to
                                 interpret relative paths in it */
-   AudacityProject *mProject;
+   AudacityProject *mProject{ GetActiveProject() };
 
    // In order to know whether or not to create a NEW window
-   bool              windowCalledOnce;
+   bool              windowCalledOnce{ false };
 
    // In order to zoom in, it must be done after files are opened
-   bool              callDurationFactor;
+   bool              callDurationFactor{ false };
-   double            durationFactor;
+   double            durationFactor{ 1 };
 
    // In order to offset scrollbar, it must be done after files are opened
-   bool              callScrollOffset;
+   bool              callScrollOffset{ false };
-   double            scrollOffset;
+   double            scrollOffset{ 0 };
 };
 
 LOFImportFileHandle::LOFImportFileHandle
@@ -168,12 +167,6 @@ LOFImportFileHandle::LOFImportFileHandle
    mTextFile(std::move(file))
    , mLOFFileName{name}
 {
-   mProject = GetActiveProject();
-   windowCalledOnce = false;
-   callDurationFactor = false;
-   durationFactor = 1;
-   callScrollOffset = false;
-   scrollOffset = 0;
 }
 
 void GetLOFImportPlugin(ImportPluginList &importPluginList,
@@ -190,6 +183,7 @@ wxString LOFImportPlugin::GetPluginFormatDescription()
 std::unique_ptr<ImportFileHandle> LOFImportPlugin::Open(const wxString &filename)
 {
    // Check if it is a binary file
+   {
       wxFile binaryFile;
       if (!binaryFile.Open(filename))
          return nullptr; // File not found
@@ -208,9 +202,7 @@ std::unique_ptr<ImportFileHandle> LOFImportPlugin::Open(const wxString &filename
             return nullptr;
          }
       }
-
+   }
-   // Close it again so it can be opened as a text file
-   binaryFile.Close();
 
    // Now open the file again as text file
    auto file = std::make_unique<wxTextFile>(filename);
@@ -235,6 +227,18 @@ auto LOFImportFileHandle::GetFileUncompressedBytes() -> ByteCount
 ProgressResult LOFImportFileHandle::Import(TrackFactory * WXUNUSED(trackFactory), TrackHolders &outTracks,
                                 Tags * WXUNUSED(tags))
 {
+   // Unlike other ImportFileHandle subclasses, this one never gives any tracks
+   // back to the caller.
+   // Instead, it recursively calls AudacityProject::Import for each file listed
+   // in the .lof file.
+   // Each importation creates a new undo state.
+   // If there is an error or exception during one of them, only that one's
+   // side effects are rolled back, and the rest of the import list is skipped.
+   // The file may have "window" directives that cause new AudacityProjects
+   // to be created, and the undo states are pushed onto the latest project.
+   // If a project is created but the first file import into it fails, destroy
+   // the project.
+
    outTracks.clear();
 
    wxASSERT(mTextFile->IsOpened());
@@ -256,15 +260,13 @@ ProgressResult LOFImportFileHandle::Import(TrackFactory * WXUNUSED(trackFactory)
    // for last line
    lofOpenFiles(&line);
 
-   // set any duration/offset factors for last window, as all files were called
+   if(!mTextFile->Close())
-   doDuration();
-   doScrollOffset();
-
-   // exited ok
-   if(mTextFile->Close())
-      return ProgressResult::Success;
-
       return ProgressResult::Failed;
+
+   // set any duration/offset factors for last window, as all files were called
+   doDurationAndScrollOffset();
+
+   return ProgressResult::Success;
 }
 
 static int CountNumTracks(AudacityProject *proj)
@@ -302,8 +304,7 @@ void LOFImportFileHandle::lofOpenFiles(wxString* ln)
    if (tokenholder.IsSameAs(wxT("window"), false))
    {
       // set any duration/offset factors for last window, as all files were called
-      doDuration();
+      doDurationAndScrollOffset();
-      doScrollOffset();
 
       if (windowCalledOnce)
       {
@@ -380,6 +381,8 @@ void LOFImportFileHandle::lofOpenFiles(wxString* ln)
          }
       }
 
+      // Do recursive call to import
+
 #ifdef USE_MIDI
       // If file is a midi
       if (targetfile.AfterLast(wxT('.')).IsSameAs(wxT("mid"), false)
@@ -459,6 +462,9 @@ void LOFImportFileHandle::lofOpenFiles(wxString* ln)
                      t->SetOffset(offset);
                   }
                }
+
+               // Amend the undo transaction made by import
+               mProject->TP_ModifyState(false);
             } // end of converting "offset" argument
             else
             {
@@ -481,23 +487,25 @@ void LOFImportFileHandle::lofOpenFiles(wxString* ln)
    }
 }
 
-void LOFImportFileHandle::doDuration()
+void LOFImportFileHandle::doDurationAndScrollOffset()
 {
+   bool doSomething = callDurationFactor || callScrollOffset;
    if (callDurationFactor)
    {
       double longestDuration = mProject->GetTracks()->GetEndTime();
       mProject->ZoomBy(longestDuration / durationFactor);
       callDurationFactor = false;
    }
-}
 
-void LOFImportFileHandle::doScrollOffset()
-{
    if (callScrollOffset && (scrollOffset != 0))
    {
       mProject->TP_ScrollWindow(scrollOffset);
       callScrollOffset = false;
    }
+
+   if (doSomething)
+      // Amend last undo state
+      mProject->TP_ModifyState(false);
 }
 
 LOFImportFileHandle::~LOFImportFileHandle()

src/import/ImportMP3.cpp

@@ -269,6 +269,10 @@ void MP3ImportFileHandle::ImportID3(Tags *tags)
 #ifdef USE_LIBID3TAG
    wxFile f;   // will be closed when it goes out of scope
    struct id3_file *fp = NULL;
+   auto cleanup = finally([&]{
+      if (fp)
+         id3_file_close(fp);
+   });
 
    if (f.Open(mFilename)) {
       // Use id3_file_fdopen() instead of id3_file_open since wxWidgets can open a
@@ -285,10 +289,8 @@ void MP3ImportFileHandle::ImportID3(Tags *tags)
    f.Detach();
 
    struct id3_tag *tp = id3_file_tag(fp);
-   if (!tp) {
+   if (!tp)
-      id3_file_close(fp);
       return;
-   }
 
    tags->Clear();
 
@@ -357,6 +359,7 @@ void MP3ImportFileHandle::ImportID3(Tags *tags)
       else if (frame->nfields == 3) {
          ustr = id3_field_getstring(&frame->fields[1]);
          if (ustr) {
+            // Is this duplication really needed?
             MallocString<> str{ (char *)id3_ucs4_utf8duplicate(ustr) };
             n = UTF8CTOWX(str.get());
          }
@@ -368,6 +371,7 @@ void MP3ImportFileHandle::ImportID3(Tags *tags)
       }
 
       if (ustr) {
+         // Is this duplication really needed?
          MallocString<> str{ (char *)id3_ucs4_utf8duplicate(ustr) };
          v = UTF8CTOWX(str.get());
       }
@@ -384,8 +388,6 @@ void MP3ImportFileHandle::ImportID3(Tags *tags)
          tags->SetTag(TAG_GENRE, tags->GetGenre(g));
       }
    }
-
-   id3_file_close(fp);
 #endif // ifdef USE_LIBID3TAG
 }
 

src/import/ImportOGG.cpp

@@ -211,7 +211,6 @@ std::unique_ptr<ImportFileHandle> OggImportPlugin::Open(const wxString &filename
       }
 
       // what to do with message?
-      file->Close();
       return nullptr;
    }
 

src/import/ImportPCM.cpp

@@ -478,6 +478,11 @@ ProgressResult PCMImportFileHandle::Import(TrackFactory *trackFactory,
          else
             block = SFCall<sf_count_t>(sf_readf_float, mFile.get(), (float *)srcbuffer.ptr(), block);
 
+         if(block < 0 || block > maxBlock) {
+            wxASSERT(false);
+            block = maxBlock;
+         }
+
          if (block) {
             auto iter = channels.begin();
             for(int c=0; c<mInfo.channels; ++iter, ++c) {
@@ -670,6 +675,7 @@ ProgressResult PCMImportFileHandle::Import(TrackFactory *trackFactory,
                else if (frame->nfields == 3) {
                   ustr = id3_field_getstring(&frame->fields[1]);
                   if (ustr) {
+                     // Is this duplication really needed?
                      MallocString<> str{ (char *)id3_ucs4_utf8duplicate(ustr) };
                      n = UTF8CTOWX(str.get());
                   }
@@ -681,6 +687,7 @@ ProgressResult PCMImportFileHandle::Import(TrackFactory *trackFactory,
                }
 
                if (ustr) {
+                  // Is this duplication really needed?
                   MallocString<> str{ (char *)id3_ucs4_utf8duplicate(ustr) };
                   v = UTF8CTOWX(str.get());
                }
@@ -700,8 +707,6 @@ ProgressResult PCMImportFileHandle::Import(TrackFactory *trackFactory,
 
             break;
          }
-
-         f.Close();
       }
    }
 #endif

src/import/ImportPlugin.h

@@ -155,6 +155,9 @@ public:
    // do the actual import, creating whatever tracks are necessary with
    // the TrackFactory and calling the progress callback every iteration
    // through the importing loop
+   // The given Tags structure may also be modified.
+   // In case of errors or exceptions, it is not necessary to leave outTracks
+   // or tags unmodified.
    virtual ProgressResult Import(TrackFactory *trackFactory, TrackHolders &outTracks,
                       Tags *tags) = 0;
 

src/import/ImportQT.cpp

@@ -245,9 +245,14 @@ ProgressResult QTImportFileHandle::Import(TrackFactory *trackFactory,
    UInt32 quality = kQTAudioRenderQuality_Max;
    AudioStreamBasicDescription desc;
    UInt32 maxSampleSize;
-   UInt32 bufsize;
    bool res = false;
 
+   auto cleanup = finally( [&] {
+      if (maer) {
+         MovieAudioExtractionEnd(maer);
+      }
+   } );
+
    CreateProgress();
 
    do
@@ -301,7 +306,7 @@ ProgressResult QTImportFileHandle::Import(TrackFactory *trackFactory,
       }
    
       auto numchan = desc.mChannelsPerFrame;
-      bufsize = 5 * desc.mSampleRate;
+      const size_t bufsize = 5 * desc.mSampleRate;
    
       // determine sample format
       sampleFormat format;
@@ -320,32 +325,38 @@ ProgressResult QTImportFileHandle::Import(TrackFactory *trackFactory,
             break;
       }
 
-      std::unique_ptr<AudioBufferList, freer> abl
+      // Allocate an array of pointers, whose size is not known statically,
-      { static_cast<AudioBufferList *>
+      // and prefixed with the AudioBufferList structure.
-         (calloc(1, offsetof(AudioBufferList, mBuffers) +
+      MallocPtr< AudioBufferList > abl{
+         static_cast< AudioBufferList * >(
+            calloc( 1, offsetof( AudioBufferList, mBuffers ) +
                (sizeof(AudioBuffer) * numchan))) };
       abl->mNumberBuffers = numchan;
    
       TrackHolders channels{ numchan };
 
-      ArraysOf<unsigned char> holders{ numchan, sizeof(float) * bufsize };
+      const auto size = sizeof(float) * bufsize;
-      int c;
+      ArraysOf<unsigned char> holders{ numchan, size };
-      for (c = 0; c < numchan; c++) {
+      for (size_t c = 0; c < numchan; c++) {
-         abl->mBuffers[c].mNumberChannels = 1;
+         auto &buffer = abl->mBuffers[c];
-         abl->mBuffers[c].mDataByteSize = sizeof(float) * bufsize;
+         auto &holder = holders[c];
+         auto &channel = channels[c];
 
-         abl->mBuffers[c].mData = holders[c].get();
+         buffer.mNumberChannels = 1;
+         buffer.mDataByteSize = size;
 
-         channels[c] = trackFactory->NewWaveTrack(format);
+         buffer.mData = holder.get();
-         channels[c]->SetRate(desc.mSampleRate);
+   
+         channel = trackFactory->NewWaveTrack( format );
+         channel->SetRate( desc.mSampleRate );
    
          if (numchan == 2) {
             if (c == 0) {
-               channels[c]->SetChannel(Track::LeftChannel);
+               channel->SetChannel(Track::LeftChannel);
-               channels[c]->SetLinked(true);
+               channel->SetLinked(true);
             }
             else if (c == 1) {
-               channels[c]->SetChannel(Track::RightChannel);
+               channel->SetChannel(Track::RightChannel);
             }
          }
       }
@@ -363,7 +374,7 @@ ProgressResult QTImportFileHandle::Import(TrackFactory *trackFactory,
             break;
          }
    
-         for (c = 0; c < numchan; c++) {
+         for (size_t c = 0; c < numchan; c++) {
             channels[c]->Append((char *) abl->mBuffers[c].mData, floatSample, numFrames);
          }
    
@@ -398,10 +409,6 @@ ProgressResult QTImportFileHandle::Import(TrackFactory *trackFactory,
 
 // done:
 
-   if (maer) {
-      MovieAudioExtractionEnd(maer);
-   }
-
    return (res ? ProgressResult::Success : ProgressResult::Failed);
 }
 
@@ -436,6 +443,12 @@ names[] =
 void QTImportFileHandle::AddMetadata(Tags *tags)
 {
    QTMetaDataRef metaDataRef = NULL;
+   auto cleanup = finally( [&] {
+      // we are done so release our metadata object
+      if ( metaDataRef )
+         QTMetaDataRelease(metaDataRef);
+   } );
+
    OSErr err;
 
    err = QTCopyMovieMetaData(mMovie, &metaDataRef);
@@ -526,9 +539,6 @@ void QTImportFileHandle::AddMetadata(Tags *tags)
       }
    }
 
-   // we are done so release our metadata object
-   QTMetaDataRelease(metaDataRef);
-
    return;
 }