FileWrapper[Impl] modifications and actually remove the "Impl" class.
This is a somewhat involved refactoring of this class. Here's an overview of the changes:
* FileWrapper can now be used as a regular class and instances allocated on the stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method. The intent of this is to allow opening a file and getting back a FileWrapper instance. Using this method instead of Create(), will allow us in the future to make the FILE* member pointer, to be const and simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface. There's only one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced. No support for looping (not clear if that was actually useful to users of that flag), no need to implement the 'read_only_' functionality in the class, since file APIs implement that already, no support for *not* managing the file handle (this wasn't used). OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text mode. Text mode is only different on Windows and on Windows it translates \n to \r\n, which means that files such as log files, could have a slightly different format on Windows than other platforms. Besides, tools on Windows can handle UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten from just checking the file pointer.
* Don't call CloseFile inside of Write. Write shouldn't be changing the state of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of both defining the Rewind method. Ultimately, Id' like to remove these interfaces and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading when checking the open_ flag.
BUG=
Review-Url: https://codereview.webrtc.org/2054373002
Cr-Commit-Position: refs/heads/master@{#13155}
diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc
index e75b328..819a18b 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -211,9 +211,7 @@
public_submodules_->gain_control_for_experimental_agc.reset();
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
- debug_dump_.debug_file->CloseFile();
- }
+ debug_dump_.debug_file->CloseFile();
#endif
}
@@ -326,7 +324,7 @@
InitializeVoiceDetection();
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
int err = WriteInitMessage();
if (err != kNoError) {
return err;
@@ -537,7 +535,7 @@
formats_.api_format.input_stream().num_frames());
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
RETURN_ON_ERR(WriteConfigMessage(false));
debug_dump_.capture.event_msg->set_type(audioproc::Event::STREAM);
@@ -555,7 +553,7 @@
capture_.capture_audio->CopyTo(formats_.api_format.output_stream(), dest);
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
const size_t channel_size =
sizeof(float) * formats_.api_format.output_stream().num_frames();
@@ -624,7 +622,7 @@
}
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
debug_dump_.capture.event_msg->set_type(audioproc::Event::STREAM);
audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
const size_t data_size =
@@ -638,7 +636,7 @@
capture_.capture_audio->InterleaveTo(frame, output_copy_needed());
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
const size_t data_size =
sizeof(int16_t) * frame->samples_per_channel_ * frame->num_channels_;
@@ -660,7 +658,7 @@
public_submodules_->echo_control_mobile->is_enabled()));
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
msg->set_delay(capture_nonlocked_.stream_delay_ms);
msg->set_drift(
@@ -828,7 +826,7 @@
formats_.api_format.reverse_input_stream().num_frames());
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
debug_dump_.render.event_msg->set_type(audioproc::Event::REVERSE_STREAM);
audioproc::ReverseStream* msg =
debug_dump_.render.event_msg->mutable_reverse_stream();
@@ -883,7 +881,7 @@
}
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
- if (debug_dump_.debug_file->Open()) {
+ if (debug_dump_.debug_file->is_open()) {
debug_dump_.render.event_msg->set_type(audioproc::Event::REVERSE_STREAM);
audioproc::ReverseStream* msg =
debug_dump_.render.event_msg->mutable_reverse_stream();
@@ -990,14 +988,9 @@
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
debug_dump_.num_bytes_left_for_log_ = max_log_size_bytes;
// Stop any ongoing recording.
- if (debug_dump_.debug_file->Open()) {
- if (debug_dump_.debug_file->CloseFile() == -1) {
- return kFileError;
- }
- }
+ debug_dump_.debug_file->CloseFile();
- if (debug_dump_.debug_file->OpenFile(filename, false) == -1) {
- debug_dump_.debug_file->CloseFile();
+ if (!debug_dump_.debug_file->OpenFile(filename, false)) {
return kFileError;
}
@@ -1023,13 +1016,9 @@
debug_dump_.num_bytes_left_for_log_ = max_log_size_bytes;
// Stop any ongoing recording.
- if (debug_dump_.debug_file->Open()) {
- if (debug_dump_.debug_file->CloseFile() == -1) {
- return kFileError;
- }
- }
+ debug_dump_.debug_file->CloseFile();
- if (debug_dump_.debug_file->OpenFromFileHandle(handle, true, false) == -1) {
+ if (!debug_dump_.debug_file->OpenFromFileHandle(handle)) {
return kFileError;
}
@@ -1057,11 +1046,7 @@
#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
// We just return if recording hasn't started.
- if (debug_dump_.debug_file->Open()) {
- if (debug_dump_.debug_file->CloseFile() == -1) {
- return kFileError;
- }
- }
+ debug_dump_.debug_file->CloseFile();
return kNoError;
#else
return kUnsupportedFunctionError;
@@ -1362,7 +1347,7 @@
// Ensure atomic writes of the message.
rtc::CritScope cs_debug(crit_debug);
- RTC_DCHECK(debug_file->Open());
+ RTC_DCHECK(debug_file->is_open());
// Update the byte counter.
if (*filesize_limit_bytes >= 0) {
*filesize_limit_bytes -=
diff --git a/webrtc/modules/audio_processing/transient/click_annotate.cc b/webrtc/modules/audio_processing/transient/click_annotate.cc
index dd1c346..b03a714 100644
--- a/webrtc/modules/audio_processing/transient/click_annotate.cc
+++ b/webrtc/modules/audio_processing/transient/click_annotate.cc
@@ -40,15 +40,15 @@
}
std::unique_ptr<FileWrapper> pcm_file(FileWrapper::Create());
- pcm_file->OpenFile(argv[1], true, false, false);
- if (!pcm_file->Open()) {
+ pcm_file->OpenFile(argv[1], true);
+ if (!pcm_file->is_open()) {
printf("\nThe %s could not be opened.\n\n", argv[1]);
return -1;
}
std::unique_ptr<FileWrapper> dat_file(FileWrapper::Create());
- dat_file->OpenFile(argv[2], false, false, false);
- if (!dat_file->Open()) {
+ dat_file->OpenFile(argv[2], false);
+ if (!dat_file->is_open()) {
printf("\nThe %s could not be opened.\n\n", argv[2]);
return -1;
}
diff --git a/webrtc/modules/audio_processing/transient/file_utils.cc b/webrtc/modules/audio_processing/transient/file_utils.cc
index ebe5306..e69bf79 100644
--- a/webrtc/modules/audio_processing/transient/file_utils.cc
+++ b/webrtc/modules/audio_processing/transient/file_utils.cc
@@ -80,7 +80,7 @@
size_t ReadInt16BufferFromFile(FileWrapper* file,
size_t length,
int16_t* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -106,7 +106,7 @@
size_t ReadInt16FromFileToFloatBuffer(FileWrapper* file,
size_t length,
float* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -124,7 +124,7 @@
size_t ReadInt16FromFileToDoubleBuffer(FileWrapper* file,
size_t length,
double* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -142,7 +142,7 @@
size_t ReadFloatBufferFromFile(FileWrapper* file,
size_t length,
float* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -165,7 +165,7 @@
size_t ReadDoubleBufferFromFile(FileWrapper* file,
size_t length,
double* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -188,7 +188,7 @@
size_t WriteInt16BufferToFile(FileWrapper* file,
size_t length,
const int16_t* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -212,7 +212,7 @@
size_t WriteFloatBufferToFile(FileWrapper* file,
size_t length,
const float* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
@@ -235,7 +235,7 @@
size_t WriteDoubleBufferToFile(FileWrapper* file,
size_t length,
const double* buffer) {
- if (!file || !file->Open() || !buffer || length <= 0) {
+ if (!file || !file->is_open() || !buffer || length <= 0) {
return 0;
}
diff --git a/webrtc/modules/audio_processing/transient/file_utils_unittest.cc b/webrtc/modules/audio_processing/transient/file_utils_unittest.cc
index 8520413..6a848c6 100644
--- a/webrtc/modules/audio_processing/transient/file_utils_unittest.cc
+++ b/webrtc/modules/audio_processing/transient/file_utils_unittest.cc
@@ -163,12 +163,9 @@
std::unique_ptr<FileWrapper> file(FileWrapper::Create());
- file->OpenFile(test_filename.c_str(),
- true, // Read only.
- true, // Loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kTestFileName.c_str();
+ file->OpenFile(test_filename.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kTestFileName.c_str();
const size_t kBufferLength = 12;
std::unique_ptr<int16_t[]> buffer(new int16_t[kBufferLength]);
@@ -207,12 +204,9 @@
std::unique_ptr<FileWrapper> file(FileWrapper::Create());
- file->OpenFile(test_filename.c_str(),
- true, // Read only.
- true, // Loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kTestFileName.c_str();
+ file->OpenFile(test_filename.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kTestFileName.c_str();
const size_t kBufferLength = 12;
std::unique_ptr<float[]> buffer(new float[kBufferLength]);
@@ -254,12 +248,9 @@
std::unique_ptr<FileWrapper> file(FileWrapper::Create());
- file->OpenFile(test_filename.c_str(),
- true, // Read only.
- true, // Loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kTestFileName.c_str();
+ file->OpenFile(test_filename.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kTestFileName.c_str();
const size_t kBufferLength = 12;
std::unique_ptr<double[]> buffer(new double[kBufferLength]);
@@ -299,12 +290,9 @@
std::unique_ptr<FileWrapper> file(FileWrapper::Create());
- file->OpenFile(test_filename.c_str(),
- true, // Read only.
- true, // Loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kTestFileNamef.c_str();
+ file->OpenFile(test_filename.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kTestFileNamef.c_str();
const size_t kBufferLength = 3;
std::unique_ptr<float[]> buffer(new float[kBufferLength]);
@@ -341,12 +329,9 @@
std::unique_ptr<FileWrapper> file(FileWrapper::Create());
- file->OpenFile(test_filename.c_str(),
- true, // Read only.
- true, // Loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kTestFileName.c_str();
+ file->OpenFile(test_filename.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kTestFileName.c_str();
const size_t kBufferLength = 3;
std::unique_ptr<double[]> buffer(new double[kBufferLength]);
@@ -384,12 +369,9 @@
std::string kOutFileName = CreateTempFilename(test::OutputPath(),
"utils_test");
- file->OpenFile(kOutFileName.c_str(),
- false, // Write mode.
- false, // No loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kOutFileName.c_str();
+ file->OpenFile(kOutFileName.c_str(), false); // Write mode.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kOutFileName.c_str();
const size_t kBufferLength = 3;
std::unique_ptr<int16_t[]> written_buffer(new int16_t[kBufferLength]);
@@ -405,12 +387,9 @@
file->CloseFile();
- file->OpenFile(kOutFileName.c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kOutFileName.c_str();
+ file->OpenFile(kOutFileName.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kOutFileName.c_str();
EXPECT_EQ(kBufferLength, ReadInt16BufferFromFile(file.get(),
kBufferLength,
@@ -431,12 +410,9 @@
std::string kOutFileName = CreateTempFilename(test::OutputPath(),
"utils_test");
- file->OpenFile(kOutFileName.c_str(),
- false, // Write mode.
- false, // No loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kOutFileName.c_str();
+ file->OpenFile(kOutFileName.c_str(), false); // Write mode.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kOutFileName.c_str();
const size_t kBufferLength = 3;
std::unique_ptr<float[]> written_buffer(new float[kBufferLength]);
@@ -452,12 +428,9 @@
file->CloseFile();
- file->OpenFile(kOutFileName.c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kOutFileName.c_str();
+ file->OpenFile(kOutFileName.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kOutFileName.c_str();
EXPECT_EQ(kBufferLength, ReadFloatBufferFromFile(file.get(),
kBufferLength,
@@ -478,12 +451,9 @@
std::string kOutFileName = CreateTempFilename(test::OutputPath(),
"utils_test");
- file->OpenFile(kOutFileName.c_str(),
- false, // Write mode.
- false, // No loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kOutFileName.c_str();
+ file->OpenFile(kOutFileName.c_str(), false); // Write mode.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kOutFileName.c_str();
const size_t kBufferLength = 3;
std::unique_ptr<double[]> written_buffer(new double[kBufferLength]);
@@ -499,12 +469,9 @@
file->CloseFile();
- file->OpenFile(kOutFileName.c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kOutFileName.c_str();
+ file->OpenFile(kOutFileName.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kOutFileName.c_str();
EXPECT_EQ(kBufferLength, ReadDoubleBufferFromFile(file.get(),
kBufferLength,
@@ -541,12 +508,9 @@
EXPECT_EQ(0u, WriteInt16BufferToFile(file.get(), 1, int16_buffer.get()));
EXPECT_EQ(0u, WriteDoubleBufferToFile(file.get(), 1, double_buffer.get()));
- file->OpenFile(test_filename.c_str(),
- true, // Read only.
- true, // Loop.
- false); // No text.
- ASSERT_TRUE(file->Open()) << "File could not be opened:\n"
- << kTestFileName.c_str();
+ file->OpenFile(test_filename.c_str(), true); // Read only.
+ ASSERT_TRUE(file->is_open()) << "File could not be opened:\n"
+ << kTestFileName.c_str();
EXPECT_EQ(0u, ReadInt16BufferFromFile(NULL, 1, int16_buffer.get()));
EXPECT_EQ(0u, ReadInt16BufferFromFile(file.get(), 1, NULL));
diff --git a/webrtc/modules/audio_processing/transient/transient_detector_unittest.cc b/webrtc/modules/audio_processing/transient/transient_detector_unittest.cc
index 18bb42e..fedddb7 100644
--- a/webrtc/modules/audio_processing/transient/transient_detector_unittest.cc
+++ b/webrtc/modules/audio_processing/transient/transient_detector_unittest.cc
@@ -53,11 +53,9 @@
detect_file->OpenFile(
test::ResourcePath(detect_file_name.str(), "dat").c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
+ true); // Read only.
- bool file_opened = detect_file->Open();
+ bool file_opened = detect_file->is_open();
ASSERT_TRUE(file_opened) << "File could not be opened.\n"
<< detect_file_name.str().c_str();
@@ -70,9 +68,7 @@
audio_file->OpenFile(
test::ResourcePath(audio_file_name.str(), "pcm").c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
+ true); // Read only.
// Create detector.
TransientDetector detector(sample_rate_hz);
diff --git a/webrtc/modules/audio_processing/transient/wpd_tree_unittest.cc b/webrtc/modules/audio_processing/transient/wpd_tree_unittest.cc
index 3e202bf..3b08314 100644
--- a/webrtc/modules/audio_processing/transient/wpd_tree_unittest.cc
+++ b/webrtc/modules/audio_processing/transient/wpd_tree_unittest.cc
@@ -95,12 +95,9 @@
std::ostringstream matlab_stream;
matlab_stream << "audio_processing/transient/wpd" << i;
std::string matlab_string = test::ResourcePath(matlab_stream.str(), "dat");
- matlab_files_data[i]->OpenFile(matlab_string.c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
+ matlab_files_data[i]->OpenFile(matlab_string.c_str(), true); // Read only.
- bool file_opened = matlab_files_data[i]->Open();
+ bool file_opened = matlab_files_data[i]->is_open();
ASSERT_TRUE(file_opened) << "File could not be opened.\n" << matlab_string;
// Out files.
@@ -110,12 +107,9 @@
out_stream << test::OutputPath() << "wpd_" << i << ".out";
std::string out_string = out_stream.str();
- out_files_data[i]->OpenFile(out_string.c_str(),
- false, // Write mode.
- false, // No loop.
- false); // No text.
+ out_files_data[i]->OpenFile(out_string.c_str(), false); // Write mode.
- file_opened = out_files_data[i]->Open();
+ file_opened = out_files_data[i]->is_open();
ASSERT_TRUE(file_opened) << "File could not be opened.\n" << out_string;
}
@@ -125,12 +119,9 @@
std::unique_ptr<FileWrapper> test_file(FileWrapper::Create());
- test_file->OpenFile(test_file_name.c_str(),
- true, // Read only.
- false, // No loop.
- false); // No text.
+ test_file->OpenFile(test_file_name.c_str(), true); // Read only.
- bool file_opened = test_file->Open();
+ bool file_opened = test_file->is_open();
ASSERT_TRUE(file_opened) << "File could not be opened.\n" << test_file_name;
float test_buffer[kTestBufferSize];