-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
compress: Fix print and log statements #181
Conversation
tools/compress/main.cpp
Outdated
@@ -106,7 +107,7 @@ int main(int argc, const char** argv) | |||
{ | |||
std::string src_compression = "NONE"; | |||
file_processor.AddDecoder(&decoder); | |||
file_processor.ProcessAllFrames(); | |||
succeeded = file_processor.ProcessAllFrames(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this failed, can the following for loop be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to check that the value returned by ProcessAllFrames is what you are expecting. It is returning the last value returned by ProcessNextFrame, which looks like it is false in all cases that would stop the replay, including success cases like eof. This looks like a bug, which could be fixed by returning something like 'number of frames processed == number of frames in file'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ProcessAllFrames always returns false bug was fixed by #194
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have amended the commit to skip the for loop on failure, as described above, with a few other minor changes.
When output resulted in a compression, we were not using the correct GFXRECON_WRITE_CONSOLE macro to output the message. We were using a standard printf. This resulted in no carraige return being added. Also, I noticed that both compress/main.cpp and format/format_util.cpp had the same error message which caused me some confusing when investigating it. So I added more details to the format_util.cpp error message.
ef79855
to
d7196a2
Compare
Fix output when an error occurs during the frame parsing process. Before, it would go on as if everything worked. Instead, we want it to be obvious that it failed.
d7196a2
to
bfb4362
Compare
tools/compress/main.cpp
Outdated
@@ -106,7 +107,7 @@ int main(int argc, const char** argv) | |||
{ | |||
std::string src_compression = "NONE"; | |||
file_processor.AddDecoder(&decoder); | |||
file_processor.ProcessAllFrames(); | |||
succeeded = file_processor.ProcessAllFrames(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have amended the commit to skip the for loop on failure, as described above, with a few other minor changes.
When output resulted in a compression, we were not using the correct
GFXRECON_WRITE_CONSOLE macro to output the message. We were using
a standard printf. This resulted in no carraige return being
added.
Also, I noticed that both compress/main.cpp and format/format_util.cpp
had the same error message which caused me some confusing when
investigating it. So I added more details to the format_util.cpp
error message.