From e1cf05671090d3219c085e9c9d3103c74b92d8f1 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sat, 20 Jan 2018 15:19:20 -0500 Subject: [PATCH] Bug1829: Inform user, erase incomplete files whenever export fails... ... Also checked more error returns from library functions. Detect it when write fails (maybe because of exhaustion of space) -- don't continue, pretending all is still well. Using one non-specific error message in many places, because we're in string freeze now. --- src/export/Export.h | 4 +++ src/export/ExportCL.cpp | 2 ++ src/export/ExportFFmpeg.cpp | 40 +++++++++++++++++++++---- src/export/ExportFLAC.cpp | 52 ++++++++++++++++++++++---------- src/export/ExportMP2.cpp | 29 ++++++++++++++---- src/export/ExportMP3.cpp | 45 +++++++++++++++++++++++----- src/export/ExportOGG.cpp | 60 +++++++++++++++++++++++++++---------- src/export/ExportPCM.cpp | 27 +++++++++++------ 8 files changed, 201 insertions(+), 58 deletions(-) diff --git a/src/export/Export.h b/src/export/Export.h index d21e11d34..e35a6940c 100644 --- a/src/export/Export.h +++ b/src/export/Export.h @@ -108,6 +108,10 @@ public: * export to "Other PCM", "AIFF 16 Bit" and "WAV 16 Bit" are all the same * libsndfile export plug-in, but with subformat set to 0, 1, and 2 * respectively. + * @return ProgressResult::Failed or ProgressResult::Cancelled if export + * fails to complete for any reason, in which case this function is + * responsible for alerting the user. Otherwise ProgressResult::Success or + * ProgressResult::Stopped */ virtual ProgressResult Export(AudacityProject *project, unsigned channels, diff --git a/src/export/ExportCL.cpp b/src/export/ExportCL.cpp index 5452aec41..c1405348f 100644 --- a/src/export/ExportCL.cpp +++ b/src/export/ExportCL.cpp @@ -529,6 +529,8 @@ ProgressResult ExportCL::Export(AudacityProject *project, dlg.Center(); dlg.ShowModal(); + + updateResult = ProgressResult::Failed; } return updateResult; diff --git a/src/export/ExportFFmpeg.cpp b/src/export/ExportFFmpeg.cpp index 7b182be09..a01a11b20 100644 --- a/src/export/ExportFFmpeg.cpp +++ b/src/export/ExportFFmpeg.cpp @@ -556,6 +556,7 @@ bool ExportFFmpeg::InitCodecs(AudacityProject *project) return true; } +// Returns 0 if no more output, 1 if more output, negative if error static int encode_audio(AVCodecContext *avctx, AVPacket *pkt, int16_t *audio_samples, int nb_samples) { // Assume *pkt is already initialized. @@ -724,9 +725,16 @@ bool ExportFFmpeg::Finalize() { AVPacketEx pkt; if (nEncodedBytes <= 0) + // We didn't encode_audio yet for this pass nEncodedBytes = encode_audio(mEncAudioCodecCtx.get(), &pkt, NULL, 0); - if (nEncodedBytes <= 0) + if (nEncodedBytes < 0) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return false; + } + + if (nEncodedBytes == 0) break; pkt.stream_index = mEncAudioStream->index; @@ -743,13 +751,23 @@ bool ExportFFmpeg::Finalize() _("FFmpeg : ERROR - Couldn't write last audio frame to output file."), _("FFmpeg Error"), wxOK | wxCENTER | wxICON_EXCLAMATION ); +#if 0 + // We ought to propagate this error, but it is known to happen + // spuriously in some versions of FFmpeg when exporting AAC + return false; +#else break; +#endif } } } // Write any file trailers. - av_write_trailer(mEncFormatCtx.get()); + if (av_write_trailer(mEncFormatCtx.get()) != 0) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return false; + } return true; } @@ -782,7 +800,8 @@ bool ExportFFmpeg::EncodeAudioFrame(int16_t *pFrame, size_t frameSize) nBytesToWrite = frameSize; pRawSamples = (uint8_t*)pFrame; - av_fifo_realloc2(mEncAudioFifo.get(), av_fifo_size(mEncAudioFifo.get()) + frameSize); + if (av_fifo_realloc2(mEncAudioFifo.get(), av_fifo_size(mEncAudioFifo.get()) + frameSize) < 0) + return false; // Put the raw audio samples into the FIFO. ret = av_fifo_generic_write(mEncAudioFifo.get(), pRawSamples, nBytesToWrite,NULL); @@ -866,8 +885,11 @@ ProgressResult ExportFFmpeg::Export(AudacityProject *project, const TrackList *tracks = project->GetTracks(); bool ret = true; - if (mSubFormat >= FMT_LAST) + if (mSubFormat >= FMT_LAST) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); return ProgressResult::Cancelled; + } wxString shortname(ExportFFmpegOptions::fmts[mSubFormat].shortname); if (mSubFormat == FMT_OTHER) @@ -875,8 +897,11 @@ ProgressResult ExportFFmpeg::Export(AudacityProject *project, ret = Init(shortname.mb_str(),project, metadata, subformat); auto cleanup = finally ( [&] { FreeResources(); } ); - if (!ret) + if (!ret) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); return ProgressResult::Cancelled; + } size_t pcmBufferSize = 1024; @@ -905,6 +930,9 @@ ProgressResult ExportFFmpeg::Export(AudacityProject *project, if (!EncodeAudioFrame( pcmBuffer, (pcmNumSamples)*sizeof(int16_t)*mChannels)) { + // TODO: more precise message, and fix redundancy with messages + // already given on some of the failure paths of the above call + AudacityMessageBox(_("Unable to export")); updateResult = ProgressResult::Cancelled; break; } @@ -914,7 +942,7 @@ ProgressResult ExportFFmpeg::Export(AudacityProject *project, } if ( updateResult != ProgressResult::Cancelled ) - if ( !Finalize() ) + if ( !Finalize() ) // Finalize makes its own messages return ProgressResult::Cancelled; return updateResult; diff --git a/src/export/ExportFLAC.cpp b/src/export/ExportFLAC.cpp index 47ee35590..0c4647a3e 100644 --- a/src/export/ExportFLAC.cpp +++ b/src/export/ExportFLAC.cpp @@ -245,22 +245,26 @@ ProgressResult ExportFLAC::Export(AudacityProject *project, FLAC::Encoder::File encoder; + bool success = true; + success = success && #ifdef LEGACY_FLAC - encoder.set_filename(OSOUTPUT(fName)); + encoder.set_filename(OSOUTPUT(fName)) && #endif - encoder.set_channels(numChannels); + encoder.set_channels(numChannels) && encoder.set_sample_rate(lrint(rate)); // See note in GetMetadata() about a bug in libflac++ 1.1.2 - if (!GetMetadata(project, metadata)) { + if (success && !GetMetadata(project, metadata)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); return ProgressResult::Cancelled; } - if (mMetadata) { + if (success && mMetadata) { // set_metadata expects an array of pointers to metadata and a size. // The size is 1. FLAC__StreamMetadata *p = mMetadata.get(); - encoder.set_metadata(&p, 1); + success = encoder.set_metadata(&p, 1); } auto cleanup1 = finally( [&] { @@ -270,32 +274,45 @@ ProgressResult ExportFLAC::Export(AudacityProject *project, sampleFormat format; if (bitDepthPref == wxT("24")) { format = int24Sample; - encoder.set_bits_per_sample(24); + success = success && encoder.set_bits_per_sample(24); } else { //convert float to 16 bits format = int16Sample; - encoder.set_bits_per_sample(16); + success = success && encoder.set_bits_per_sample(16); } + // Duplicate the flac command line compression levels if (levelPref < 0 || levelPref > 8) { levelPref = 5; } - encoder.set_do_exhaustive_model_search(flacLevels[levelPref].do_exhaustive_model_search); + success = success && + encoder.set_do_exhaustive_model_search(flacLevels[levelPref].do_exhaustive_model_search) && encoder.set_do_escape_coding(flacLevels[levelPref].do_escape_coding); + if (numChannels != 2) { - encoder.set_do_mid_side_stereo(false); + success = success && + encoder.set_do_mid_side_stereo(false) && encoder.set_loose_mid_side_stereo(false); } else { - encoder.set_do_mid_side_stereo(flacLevels[levelPref].do_mid_side_stereo); + success = success && + encoder.set_do_mid_side_stereo(flacLevels[levelPref].do_mid_side_stereo) && encoder.set_loose_mid_side_stereo(flacLevels[levelPref].loose_mid_side_stereo); } - encoder.set_qlp_coeff_precision(flacLevels[levelPref].qlp_coeff_precision); - encoder.set_min_residual_partition_order(flacLevels[levelPref].min_residual_partition_order); - encoder.set_max_residual_partition_order(flacLevels[levelPref].max_residual_partition_order); - encoder.set_rice_parameter_search_dist(flacLevels[levelPref].rice_parameter_search_dist); + + success = success && + encoder.set_qlp_coeff_precision(flacLevels[levelPref].qlp_coeff_precision) && + encoder.set_min_residual_partition_order(flacLevels[levelPref].min_residual_partition_order) && + encoder.set_max_residual_partition_order(flacLevels[levelPref].max_residual_partition_order) && + encoder.set_rice_parameter_search_dist(flacLevels[levelPref].rice_parameter_search_dist) && encoder.set_max_lpc_order(flacLevels[levelPref].max_lpc_order); + if (!success) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } + #ifdef LEGACY_FLAC encoder.init(); #else @@ -361,6 +378,8 @@ ProgressResult ExportFLAC::Export(AudacityProject *project, if (! encoder.process( reinterpret_cast( tmpsmplbuf.get() ), samplesThisRun) ) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); updateResult = ProgressResult::Cancelled; break; } @@ -402,9 +421,10 @@ bool ExportFLAC::GetMetadata(AudacityProject *project, const Tags *tags) } FLAC::Metadata::VorbisComment::Entry entry(n.mb_str(wxConvUTF8), v.mb_str(wxConvUTF8)); - ::FLAC__metadata_object_vorbiscomment_append_comment(mMetadata.get(), + if (! ::FLAC__metadata_object_vorbiscomment_append_comment(mMetadata.get(), entry.get_entry(), - true); + true) ) + return false; } return true; diff --git a/src/export/ExportMP2.cpp b/src/export/ExportMP2.cpp index cae2b7dcb..4fd603ea5 100644 --- a/src/export/ExportMP2.cpp +++ b/src/export/ExportMP2.cpp @@ -245,8 +245,13 @@ ProgressResult ExportMP2::Export(AudacityProject *project, int id3len; bool endOfFile; id3len = AddTags(project, id3buffer, &endOfFile, metadata); - if (id3len && !endOfFile) - outFile.Write(id3buffer.get(), id3len); + if (id3len && !endOfFile) { + if ( outFile.Write(id3buffer.get(), id3len).GetLastError() ) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } + } // Values taken from the twolame simple encoder sample const size_t pcmBufferSize = 9216 / 2; // number of samples @@ -288,11 +293,17 @@ ProgressResult ExportMP2::Export(AudacityProject *project, mp2BufferSize); if (mp2BufferNumBytes < 0) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); updateResult = ProgressResult::Cancelled; break; } - outFile.Write(mp2Buffer.get(), mp2BufferNumBytes); + if ( outFile.Write(mp2Buffer.get(), mp2BufferNumBytes).GetLastError() ) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); } @@ -304,12 +315,20 @@ ProgressResult ExportMP2::Export(AudacityProject *project, mp2BufferSize); if (mp2BufferNumBytes > 0) - outFile.Write(mp2Buffer.get(), mp2BufferNumBytes); + if ( outFile.Write(mp2Buffer.get(), mp2BufferNumBytes).GetLastError() ) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } /* Write ID3 tag if it was supposed to be at the end of the file */ if (id3len && endOfFile) - outFile.Write(id3buffer.get(), id3len); + if ( outFile.Write(id3buffer.get(), id3len).GetLastError() ) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } outFile.Close(); diff --git a/src/export/ExportMP3.cpp b/src/export/ExportMP3.cpp index 34963fc63..8c0b2a60f 100644 --- a/src/export/ExportMP3.cpp +++ b/src/export/ExportMP3.cpp @@ -1801,7 +1801,11 @@ ProgressResult ExportMP3::Export(AudacityProject *project, bool endOfFile; id3len = AddTags(project, id3buffer, &endOfFile, metadata); if (id3len && !endOfFile) { - outFile.Write(id3buffer.get(), id3len); + if (id3len < outFile.Write(id3buffer.get(), id3len)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } } wxFileOffset pos = outFile.Tell(); @@ -1809,8 +1813,11 @@ ProgressResult ExportMP3::Export(AudacityProject *project, long bytes; size_t bufferSize = std::max(0, exporter.GetOutBufferSize()); - if (bufferSize == 0) + if (bufferSize <= 0) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); return ProgressResult::Cancelled; + } ArrayOf buffer{ bufferSize }; wxASSERT(buffer); @@ -1880,22 +1887,42 @@ ProgressResult ExportMP3::Export(AudacityProject *project, break; } - outFile.Write(buffer.get(), bytes); + if (bytes < outFile.Write(buffer.get(), bytes)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + updateResult = ProgressResult::Cancelled; + break; + } updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); } } - if ( updateResult != ProgressResult::Cancelled ) { + if ( updateResult == ProgressResult::Success || + updateResult == ProgressResult::Stopped ) { bytes = exporter.FinishStream(buffer.get()); + if (bytes < 0) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } + if (bytes > 0) { - outFile.Write(buffer.get(), bytes); + if (bytes < outFile.Write(buffer.get(), bytes)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } } // Write ID3 tag if it was supposed to be at the end of the file if (id3len > 0 && endOfFile) { - outFile.Write(id3buffer.get(), id3len); + if (bytes < outFile.Write(id3buffer.get(), id3len)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } } // Always write the info (Xing/Lame) tag. Until we stop supporting Lame @@ -1906,7 +1933,11 @@ ProgressResult ExportMP3::Export(AudacityProject *project, // this call, so do not use it. exporter.PutInfoTag(outFile, pos); - outFile.Close(); + if (!outFile.Close()) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } } return updateResult; diff --git a/src/export/ExportOGG.cpp b/src/export/ExportOGG.cpp index f15e4394c..0a71fb2e5 100644 --- a/src/export/ExportOGG.cpp +++ b/src/export/ExportOGG.cpp @@ -202,24 +202,41 @@ ProgressResult ExportOGG::Export(AudacityProject *project, vorbis_comment_clear(&comment); } ); + // Many of the library functions called below return 0 for success and + // various nonzero codes for failure. + // Encoding setup vorbis_info_init(&info); - vorbis_encode_init_vbr(&info, numChannels, (int)(rate + 0.5), quality); + if (vorbis_encode_init_vbr(&info, numChannels, (int)(rate + 0.5), quality)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } // Retrieve tags if (!FillComment(project, &comment, metadata)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); return ProgressResult::Cancelled; } // Set up analysis state and auxiliary encoding storage - vorbis_analysis_init(&dsp, &info); - vorbis_block_init(&dsp, &block); + if (vorbis_analysis_init(&dsp, &info) || + vorbis_block_init(&dsp, &block)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } // Set up packet->stream encoder. According to encoder example, // a random serial number makes it more likely that you can make // chained streams with concatenation. srand(time(NULL)); - ogg_stream_init(&stream, rand()); + if (ogg_stream_init(&stream, rand())) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } // First we need to write the required headers: // 1. The Ogg bitstream header, which contains codec setup params @@ -233,19 +250,26 @@ ProgressResult ExportOGG::Export(AudacityProject *project, ogg_packet comment_header; ogg_packet codebook_header; - vorbis_analysis_headerout(&dsp, &comment, &bitstream_header, &comment_header, - &codebook_header); - - // Place these headers into the stream - ogg_stream_packetin(&stream, &bitstream_header); - ogg_stream_packetin(&stream, &comment_header); - ogg_stream_packetin(&stream, &codebook_header); + if(vorbis_analysis_headerout(&dsp, &comment, &bitstream_header, &comment_header, + &codebook_header) || + // Place these headers into the stream + ogg_stream_packetin(&stream, &bitstream_header) || + ogg_stream_packetin(&stream, &comment_header) || + ogg_stream_packetin(&stream, &codebook_header)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } // Flushing these headers now guarentees that audio data will // start on a NEW page, which apparently makes streaming easier while (ogg_stream_flush(&stream, &page)) { - outFile.Write(page.header, page.header_len); - outFile.Write(page.body, page.body_len); + if ( outFile.Write(page.header, page.header_len).GetLastError() || + outFile.Write(page.body, page.body_len).GetLastError()) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } } const WaveTrackConstArray waveTracks = @@ -310,8 +334,12 @@ ProgressResult ExportOGG::Export(AudacityProject *project, break; } - outFile.Write(page.header, page.header_len); - outFile.Write(page.body, page.body_len); + if ( outFile.Write(page.header, page.header_len).GetLastError() || + outFile.Write(page.body, page.body_len).GetLastError()) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } if (ogg_page_eos(&page)) { eos = 1; @@ -322,6 +350,8 @@ ProgressResult ExportOGG::Export(AudacityProject *project, if (err) { updateResult = ProgressResult::Cancelled; + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); break; } diff --git a/src/export/ExportPCM.cpp b/src/export/ExportPCM.cpp index 021dec4aa..50641950a 100644 --- a/src/export/ExportPCM.cpp +++ b/src/export/ExportPCM.cpp @@ -529,18 +529,27 @@ ProgressResult ExportPCM::Export(AudacityProject *project, } // Install the WAV metata in a "LIST" chunk at the end of the file - if ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAV || - (sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAVEX) { - if (!AddStrings(project, sf.get(), metadata, sf_format)) { - return ProgressResult::Cancelled; - } + if (updateResult == ProgressResult::Success || + updateResult == ProgressResult::Stopped) + if ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAV || + (sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAVEX) { + if (!AddStrings(project, sf.get(), metadata, sf_format)) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } } } - if (((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_AIFF) || - ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAV)) - if (!AddID3Chunk(fName, metadata, sf_format) ) - return ProgressResult::Cancelled; + if (updateResult == ProgressResult::Success || + updateResult == ProgressResult::Stopped) + if (((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_AIFF) || + ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAV)) + if (!AddID3Chunk(fName, metadata, sf_format) ) { + // TODO: more precise message + AudacityMessageBox(_("Unable to export")); + return ProgressResult::Cancelled; + } return updateResult; }