Skip to content
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

Stream output file to stdout #1326

Merged
merged 4 commits into from
Mar 24, 2024
Merged

Stream output file to stdout #1326

merged 4 commits into from
Mar 24, 2024

Conversation

Meakk
Copy link
Contributor

@Meakk Meakk commented Mar 22, 2024

Implement #1101

@Meakk Meakk self-assigned this Mar 22, 2024
@Meakk Meakk requested a review from mwestphal March 22, 2024 20:18
@Meakk
Copy link
Contributor Author

Meakk commented Mar 22, 2024

@mwestphal @snoyer please take a look :)

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.45%. Comparing base (5055e64) to head (7250664).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1326   +/-   ##
=======================================
  Coverage   96.44%   96.45%           
=======================================
  Files         146      146           
  Lines        8746     8765   +19     
=======================================
+ Hits         8435     8454   +19     
  Misses        311      311           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@snoyer snoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key point here is that we want to ensure nothing has been written to stdout before we use it to output render data, otherwise we will quietly output a corrupted PNG.

Setting the log level to ERROR early seemed to do a good enough job when I suggested it but I haven't fully tested it.
Possible issues include:

  • if ERROR level messages go to stdout and can we have non-critical error messages (ie. not exceptions and we still proceed to render)
  • if there's any code that output to stdout directly (most likely external code not going through f3d::log)
  • if there are cases where you'd want to render to stdout and still see log messages somehow

Maybe it'd be better to not mess with the verbosity levels and instead redirect stdout to stderr from the start of the app if we know we'll be rendering to stdout?
That way any logging would still get to the terminal through stderr and only the PNG data would go to stdout (f3d foo.bar --verbose --output=- > /tmp/f3d.png would output both the full log and a valid PNG).
Would that be just a dirty hack? Are there any critical implications I'm not seeing?

application/F3DStarter.cxx Outdated Show resolved Hide resolved
application/F3DStarter.cxx Outdated Show resolved Hide resolved
Copy link

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: python, java, webassembly.

@Meakk Meakk merged commit eb2d844 into f3d-app:master Mar 24, 2024
42 checks passed
@Meakk Meakk deleted the stream-out branch March 24, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants