Re: [Audacity-devel] Bug 137
A free multi-track audio editor and recorder
Brought to you by:
aosiniao
From: Michael C. <mc...@gm...> - 2011-08-10 00:43:25
|
On Thu, Aug 4, 2011 at 5:23 PM, Edgar <edg...@wa...> wrote: > > Martyn Shaw-3 wrote: >> >> >> Agreed, first 'Write' error should stops everything, for now. I still >> think that we should try again a finite few times, just in case it's >> transient. >> > > As this (writeError not discFull) code is intended only to be there to help > pin down the source of bug137 and the user would be notified of both the > error and the fact that the Project failed to save, adding a logic change in > this patch might be less than optimal. Maybe the Write() error messages > should be expanded to include a mention of bug137, a suggestion that the > problem may be transient and that the user should try the operation again. > > I think that adding such a retry makes some sense, especially if we find > this to be a cause of bug137. Unfortunately, the way to do it seems to be in > wxWidgets or by creating our own global Read()/Write() functions with > built-in retry of the wxWidgets ones. I looked at the patch at: http://audacity.238276.n2.nabble.com/file/n6639223/exhaustiveWriteReturn1.patch Thanks for the hard work. Main notes: 1. As martyn said not having a message box for each fail occurrance, like the handling of missing aliases (which probably still needs work, but is mostly there and the dialog code can be abstracted/refactored to handle this case as well) 2. ODPCMAliasBlockFile and possibly other code uses fwrite instead of the wx write methods to be thread safe. You don't have to handle this unless you want to be exhaustive. I'm just noting it in case it slipped by you. 3. It might be good to use macros given the amount of code repetition. See the comments in SimpleBlockFile.cpp for a macro suggestion. I think gui thread issues (we can't call gui code like wxMessageBox from non-main threads) is okay, but I'm not certain.Note that refactoring the missing aliased dialog code would handle this. >> Storing up errors like these is an extremely bad idea. The very first one >> makes the data in the Project "bad". The only reasonable solution is to trap >> the error (notifying the user of the specific error), do the appropriate >> recovery (e. g. in a SaveAs Project roll back any already written AUs and >> the AUP if it is partially finished, delete the empty _data folders and >> notify the user that the SaveAs failed). This is the code I have presented >> in discFull but conceptually it is way too involved for pre-2.0 unless a QA >> decision is made and major Developer support exists. > > Agreed, first 'Write' error should stops everything, for now. I still > think that we should try again a finite few times, just in case it's > transient. I agree that once a write has failed we should abort and rollback the current process. However, this is different from accumulating error messages into a non-modal dialog. If a write fails it doesn't mean that audacity can't continue being useful to the user, because there are actions such as playback that do not require writes to happen. With things like autosave and the undo stack a modal dialog can make the UI competely unusable, whereas if the errors from distinct processes (each is aborted/returned/rollback'ed on error) are 'accumulated' and displayed in a non modal dialog the user can still use some of these functions. Also as I mentioned I don't think threads are an issue but if we do exhaustive write checking then they will be. In that case we do need to 'accumulate' errors in the sense that we can't display a message box on the non main thread. > >> Every change is wrapped in "//efm5 start" / "//efm5 end" or, if a single >> line or two, is identified with efm5. "efm5" exists no where else in the >> code. It would be a matter of a few minutes work to remove (or find those >> areas which need extend-to-recovery) all this code having the original patch >> as a reference. This code is not experimental and wrapping it in such would >> be a chore and waste of time. Feel free to make that change or any other. > > OK, I'm convinced. We can identify all of the changed code by 'efm5'. I don't think we need the efm5, because in many cases it isn't useful. Here's a more detailed review: There are some cosmetic comments, but these are mostly for future patches. If you aren't using something like git then I understand it is a lot of work to split patches this large. On Wed, Aug 10, 2011 at 12:26 AM, Michael Chinen <mc...@gm...> wrote: > [...] > Index: src/BlockFile.cpp > =================================================================== > --- src/BlockFile.cpp (revision 11226) > +++ src/BlockFile.cpp (working copy) > @@ -202,7 +202,10 @@ > sampleFormat format) > { > if(fullSummary)delete[] fullSummary; > + //efm5 This "new" might need to be in a try/catch > + // Or at least some form of error checking > fullSummary = new char[mSummaryInfo.totalSummaryBytes]; > + if (! fullSummary) return fullSummary;//efm5 > > memcpy(fullSummary, headerTag, headerTagLen); > > @@ -581,9 +584,17 @@ > samplePtr sampleData = NewSamples(mLen, floatSample); > this->ReadData(sampleData, floatSample, 0, mLen); > > - void *summaryData = BlockFile::CalcSummary(sampleData, mLen, > - floatSample); > - summaryFile.Write(summaryData, mSummaryInfo.totalSummaryBytes); > + //efm5 there is a chance that the following statement could fail > + // because it returns this fullSummary: > + void *summaryData = BlockFile::CalcSummary(sampleData, mLen, floatSample); > + if (!summaryData){ > + wxMessageBox(_("CalcSummary error – returned NULL in > AliasBlockFile::WriteSummary")); /*efm5*/ > + DeleteSamples(sampleData); > + return; > + } > + size_t wroteLength = mSummaryInfo.totalSummaryBytes; > + if (summaryFile.Write(summaryData, wroteLength) != wroteLength) > + wxMessageBox(_("file write error in > AliasBlockFile::WriteSummary")); /*efm5*/ > > DeleteSamples(sampleData); > } this code corresponds to ODPCMAliasBlockFile::CalcSummary > Index: src/blockfile/SimpleBlockFile.cpp > =================================================================== > --- src/blockfile/SimpleBlockFile.cpp (revision 11226) > +++ src/blockfile/SimpleBlockFile.cpp (working copy) > @@ -218,28 +218,54 @@ > if (!summaryData) > summaryData = /*BlockFile::*/CalcSummary(sampleData, sampleLen, > format); //mchinen:allowing virtual override of calc summary for > ODDecodeBlockFile. > > - file.Write(&header, sizeof(header)); > - file.Write(summaryData, mSummaryInfo.totalSummaryBytes); > + if (!summaryData) { > + wxMessageBox(_("CalcSummary error – returned NULL in > SimpleBlockFile")); /*efm5*/ > + return false; > + } > + > + size_t wroteLength = sizeof(header); > + if (file.Write(&header, wroteLength) != wroteLength) { > + wxMessageBox(wxString::Format(_("file write error in > SimpleBlockFile %d"), 1)); /*efm5*/ > + return false; > + } > > + wroteLength = mSummaryInfo.totalSummaryBytes; > + if (file.Write(summaryData, wroteLength) != wroteLength) { > + wxMessageBox(wxString::Format(_("file write error in > SimpleBlockFile %d"), 2)); /*efm5*/ > + return false; > + } > + > + > if( format == int24Sample ) > { > // we can't write the buffer directly to disk, because 24-bit samples > // on disk need to be packed, not padded to 32 bits like they are in > // memory > int *int24sampleData = (int*)sampleData; > + bool writeFailure = false; not needed? see below comment > > - for( int i = 0; i < sampleLen; i++ ) > + for( int i = 0; i < sampleLen; i++ ) { > #if wxBYTE_ORDER == wxBIG_ENDIAN > - file.Write((char*)&int24sampleData[i] + 1, 3); > + if (file.Write((char*)&int24sampleData[i] + 1, 3) != 3) { > #else > - file.Write((char*)&int24sampleData[i], 3); > + if (file.Write((char*)&int24sampleData[i], 3) != 3) { > #endif > + wxMessageBox(wxString::Format(_("file write error in > SimpleBlockFile %d"), 3)); /*efm5*/ > + i = sampleLen; > + writeFailure = true; I don't see a reason not to just return false here instead of these two lines > + } > + } > + if (writeFailure) return false; not needed? > } > else > { > // for all other sample formats we can write straight from the buffer > // to disk > - file.Write(sampleData, sampleLen * SAMPLE_SIZE(format)); > + wroteLength = sampleLen * SAMPLE_SIZE(format); > + if (file.Write(sampleData, wroteLength) != wroteLength) { > + wxMessageBox(wxString::Format(_("file write error in > SimpleBlockFile %d"), 4)); /*efm5*/ > + return false; > + } There are many occurrences of this pattern. Maybe it would be worth having a macro that does these four lines in one? something like #define WRITE_DATA_CHK(file, ptr, len, warningString, HANDLER) \ if (file.Write(ptr, len) != len) {\ wxMessageBox(warningString);\ HANDLER;\ }\ Then for the above code you would replace the 4 lines with WRITE_DATA_CHK(file,sampleData,wroteLength,wxString::Format(_("file write error in SimpleBlockFile %d"), 4), return false); The HANDLER parameter of the macro can be used for cleanup/returning. Although as a rule of thumb its ugly to return from within a macro this method makes the return keyword visible and also allows for other cleanup methods. You would need one macro for each write type (pointer, char, string). Significant work but would make this patch prettier and provide us with a useful macro that is less error prone than typing the check out each time. However I do not think it is so good to have a macro be defined to return a value (because it is a hidden return and also some cases you need to cleanup previous code on fail). So it may be best to have macro evaluate to an error value, check that and return. Your resulting client code would still be a one line if statement. Also, just to note - there are many cases where we return earlier on a fail - have you verified that each of these do not require cleanup of prior code? > } > > DEBUG_OUTPUT("Wrote simple block file"); > @@ -557,14 +583,25 @@ > header.encoding = AU_SAMPLE_FORMAT_16; > header.sampleRate = 44100; > header.channels = 1; > - file.Write(&header, sizeof(header)); > + size_t wroteLength = sizeof(header); > + if (file.Write(&header, wroteLength) != wroteLength) { > + wxMessageBox(_("file write error in SimpleBlockFile::Recover > 1")); /*efm5*/ > + return; > + } > > - for(i=0;i<mSummaryInfo.totalSummaryBytes;i++) > - file.Write(wxT("\0"),1); > + for(i = 0; i < mSummaryInfo.totalSummaryBytes; i++) { > + if (file.Write(wxT("\0"), 1) != 1) { > + wxMessageBox(_("file write error in SimpleBlockFile::Recover > 2")); /*efm5*/ > + return; > + } > + } > > - for(i=0;i<mLen*2;i++) > - file.Write(wxT("\0"),1); > - > + for(i = 0; i < mLen * 2; i++) { > + if (file.Write(wxT("\0"), 1)) { > + wxMessageBox(_("file write error in SimpleBlockFile::Recover > 3")); /*efm5*/ > + return; > + } > + } > } > > void SimpleBlockFile::WriteCacheToDisk() > Index: src/effects/Contrast.cpp > =================================================================== > --- src/effects/Contrast.cpp (revision 11226) > +++ src/effects/Contrast.cpp (working copy) > @@ -567,11 +567,13 @@ > f.AddLine(wxT("===================================")); > f.AddLine(wxT("")); > > + > #ifdef __WXMAC__ > - f.Write(wxTextFileType_Mac); > + if (!f.Write(wxTextFileType_Mac)) > #else > - f.Write(); > + if (!f.Write()) > #endif > + wxMessageBox(_("file write error in > ContrastDialog::OnExport")); /*efm5*/ ok, but we don't need efm5 in comments. > f.Close(); > } > > Index: src/effects/NoiseRemoval.cpp > =================================================================== > --- src/effects/NoiseRemoval.cpp (revision 11226) > +++ src/effects/NoiseRemoval.cpp (working copy) > @@ -190,9 +190,9 @@ > wxFFile noiseGateFile(fileName, wxT("wb")); > bool flag = noiseGateFile.IsOpened(); > if (flag == true) { > - int expectedCount = (mWindowSize / 2) * sizeof(float); > + size_t expectedCount = (mWindowSize / 2) * sizeof(float); Is this necessary for this patch? If it is just fixing a warning, it should be a different patch to minimize risk. > // FIX-ME: Should we check return value on Write? > - noiseGateFile.Write(mNoiseThreshold, expectedCount); > + if (noiseGateFile.Write(mNoiseThreshold, expectedCount) != > expectedCount) wxMessageBox(_("file write error in > EffectNoiseRemoval::CleanSpeechMayWriteNoiseGate")); /*efm5*/ ditto > noiseGateFile.Close(); > } > else { > Index: src/effects/VST/VSTEffect.cpp > =================================================================== > --- src/effects/VST/VSTEffect.cpp (revision 11226) > +++ src/effects/VST/VSTEffect.cpp (working copy) > @@ -1164,10 +1164,11 @@ > } > else if (Load()) { > pm.RegisterPlugin(VSTPLUGINTYPE, mPath); > - pm.Write(wxT("Name"), mName); > - pm.Write(wxT("Vendor"), mVendor); > - pm.Write(wxT("Inputs"), mInputs); > - pm.Write(wxT("Outputs"), mOutputs); > + bool goodWrite = pm.Write(wxT("Name"), mName); > + if (goodWrite) goodWrite = pm.Write(wxT("Vendor"), mVendor); > + if (goodWrite) goodWrite = pm.Write(wxT("Inputs"), mInputs); > + if (goodWrite) goodWrite = pm.Write(wxT("Outputs"), mOutputs); > + if (!goodWrite) wxMessageBox(_("file write error in > VSTEffect::VSTEffect")); /*efm5*/ > } > > if (mVendor.IsEmpty()) { > Index: src/export/ExportCL.cpp > =================================================================== > --- src/export/ExportCL.cpp (revision 11226) > +++ src/export/ExportCL.cpp (working copy) > @@ -401,7 +401,8 @@ > > // write the header > wxOutputStream *os = p->GetOutputStream(); > - os->Write(&header, sizeof(wav_header)); > + wxOutputStream & rOS1 = os->Write(&header, sizeof(wav_header)); > + if (rOS1.GetLastError() == wxSTREAM_WRITE_ERROR) > wxMessageBox(_("file write error in ExportCL::Export 1")); /*efm5*/ This is a different style but could still benefit from a macro. Many cases in the exporters are not returned immediately - Is this always due to cleanup? > > // Mix 'em up > int numWaveTracks; > @@ -463,7 +464,8 @@ > numBytes -= bytes; > > while (bytes > 0) { > - os->Write(mixed, bytes); > + wxOutputStream & rOS2 = os->Write(mixed, bytes); > + if (rOS2.GetLastError() == wxSTREAM_WRITE_ERROR) > wxMessageBox(_("file write error in ExportCL::Export 2")); /*efm5*/ > if (!os->IsOk()) { > break; > } > Index: src/export/ExportFFmpeg.cpp > =================================================================== > --- src/export/ExportFFmpeg.cpp (revision 11226) > +++ src/export/ExportFFmpeg.cpp (working copy) > @@ -327,6 +327,7 @@ > if ((err = av_write_header(mEncFormatCtx)) < 0) > { > wxLogError(wxT("FFmpeg : ERROR - Can't write headers to output > file \"%s\". Error code is %d."), mName.c_str(),err); > + wxMessageBox(wxT("FFmpeg : ERROR - Can't write headers to > output file \"%s\". Error code is %d."), mName.c_str(),err); > > return false; > } > @@ -662,6 +663,7 @@ > if ((ret = av_interleaved_write_frame(mEncFormatCtx, &pkt)) != 0) > { > wxLogError(wxT("FFmpeg : ERROR - Failed to write audio frame > to file.")); > + wxMessageBox(wxT("FFmpeg : ERROR - Failed to write audio > frame to file.")); > return false; > } > } > Index: src/export/ExportFFmpegDialogs.cpp > =================================================================== > --- src/export/ExportFFmpegDialogs.cpp (revision 11226) > +++ src/export/ExportFFmpegDialogs.cpp (working copy) > @@ -735,21 +735,29 @@ > > void FFmpegPresets::WriteXMLHeader(XMLWriter &xmlFile) > { > - xmlFile.Write(wxT("<?xml ")); > - xmlFile.Write(wxT("version=\"1.0\" ")); > - xmlFile.Write(wxT("standalone=\"no\" ")); > - xmlFile.Write(wxT("?>\n")); > + //start efm5 > + try { > + xmlFile.Write(wxT("<?xml ")); > + xmlFile.Write(wxT("version=\"1.0\" ")); > + xmlFile.Write(wxT("standalone=\"no\" ")); > + xmlFile.Write(wxT("?>\n")); > > - wxString dtdName = wxT("-//audacityffmpegpreset-1.0.0//DTD//EN"); > - wxString dtdURI = > - wxT("http://audacity.sourceforge.net/xml/audacityffmpegpreset-1.0.0.dtd"); > + wxString dtdName = wxT("-//audacityffmpegpreset-1.0.0//DTD//EN"); > + wxString dtdURI = > + wxT("http://audacity.sourceforge.net/xml/audacityffmpegpreset-1.0.0.dtd"); > > - xmlFile.Write(wxT("<!DOCTYPE ")); > - xmlFile.Write(wxT("project ")); > - xmlFile.Write(wxT("PUBLIC ")); > - xmlFile.Write(wxT("\"-//audacityffmpegpreset-1.0.0//DTD//EN\" ")); > - xmlFile.Write(wxT("\"http://audacity.sourceforge.net/xml/audacityffmpegpreset-1.0.0.dtd\" > ")); > - xmlFile.Write(wxT(">\n")); > + xmlFile.Write(wxT("<!DOCTYPE ")); > + xmlFile.Write(wxT("project ")); > + xmlFile.Write(wxT("PUBLIC ")); > + xmlFile.Write(wxT("\"-//audacityffmpegpreset-1.0.0//DTD//EN\" ")); > + xmlFile.Write(wxT("\"http://audacity.sourceforge.net/xml/audacityffmpegpreset-1.0.0.dtd\" > ")); > + xmlFile.Write(wxT(">\n")); > + } > + catch (XMLFileWriterException* pException) { > + wxMessageBox(wxString::Format(_("Couldn't write FFmpegPresets > XML header: %s"), pException->GetMessage().c_str()), _("Error writing > FFmpegPresets XML header"), wxICON_ERROR); /*efm5*/ > + delete pException; > + } > + //efm5 end removable scruff > } > > void FFmpegPresets::WriteXML(XMLWriter &xmlFile) > [...] > =================================================================== > --- src/export/ExportPCM.cpp (revision 11226) > +++ src/export/ExportPCM.cpp (working copy) > @@ -727,14 +727,15 @@ > > sz = wxUINT32_SWAP_ON_LE((wxUint32) len); > f.SeekEnd(0); > - f.Write("ID3 ", 4); > - f.Write(&sz, 4); > + if (f.Write("ID3 ", 4) != 4) wxMessageBox(_("file write error > in ExportPCM::AddID3Chunk 1")); /*efm5*/ > + if (f.Write(&sz, 4) != 4) wxMessageBox(_("file write error in > ExportPCM::AddID3Chunk 2")); /*efm5*/ > > - f.Write(buffer, len + (len & 0x01)); > + size_t wroteLength = len + (len & 0x01); > + if (f.Write(buffer, wroteLength) != wroteLength) > wxMessageBox(_("file write error in ExportPCM::AddID3Chunk 3")); > /*efm5*/ > > sz = wxUINT32_SWAP_ON_LE((wxUint32) f.Tell() - 8); > f.Seek(4); > - f.Write(&sz, 4); > + if (f.Write(&sz, 4) != 4) wxMessageBox(_("file write error in > ExportPCM::AddID3Chunk 4")); /*efm5*/ > > f.Close(); > } > Index: src/FFmpeg.cpp > =================================================================== > --- src/FFmpeg.cpp (revision 11226) > +++ src/FFmpeg.cpp (working copy) > @@ -215,7 +215,7 @@ > static int ufile_write(URLContext *h, const unsigned char *buf, int size) > #endif > { > - return (int) ((wxFile *) h->priv_data)->Write(buf, size); > + return (int) ((wxFile *) h->priv_data)->Write(buf, size);//efm5 > maybe this should return size_t > } > Index: src/Menus.cpp > =================================================================== > --- src/Menus.cpp (revision 11226) > +++ src/Menus.cpp (working copy) > @@ -2934,10 +2934,11 @@ > } > > #ifdef __WXMAC__ > - f.Write(wxTextFileType_Mac); > + if (!f.Write(wxTextFileType_Mac)) > #else > - f.Write(); > + if (!f.Write()) > #endif > + wxMessageBox(wxString::Format(_("file write error in Menus > %d"), 1)); /*efm5*/ > f.Close(); > } > > @@ -5573,9 +5574,17 @@ > preset[13] = noiseCheckSum; > gPrefs->Write(wxT("/Validate/NoiseGateSum"), noiseCheckSum); > > - int lenPreset = sizeof(preset); > - int count = presetsFile.Write(preset, lenPreset); > - count = presetsFile.Write(pNoiseGate, expectedCount); > + int wroteLength = sizeof(preset); > + if (presetsFile.Write(preset, wroteLength) != wroteLength){ > + wxMessageBox(wxString::Format(_("file write error in > Menus %d"), 2)); /*efm5*/ > + presetsFile.Close(); > + return; > + } > + if (presetsFile.Write(pNoiseGate, expectedCount) != expectedCount){ > + wxMessageBox(wxString::Format(_("file write error in > Menus %d"), 3)); /*efm5*/ > + presetsFile.Close(); > + return; not needed. > + } > > presetsFile.Close(); > } > [...] Michael |