Expose checkpoint option and save/restore renders in cli #2120

oktomus · 2018-07-12T07:51:19Z

Future Work

Expose option in studio
Save denoiser checkpoint in the same exr file
Allow to resume progressive rendering
Suggested by @est77 : Add checkpoint paths in the project file
Handle interruption happening while writing a checkpoint file (a solution could be to stop Ctrl-C during that moment).

#2355

dictoon · 2018-08-29T13:06:27Z

Will need to be rebased.

dictoon

The code is very well written and looks extremely clean.

dictoon · 2018-08-29T13:08:56Z

src/appleseed.cli/commandlinehandler.cpp

+    parser().add_option_handler(
+        &m_checkpoint
+            .add_name("--checkpoint")
+            .set_description("save a rendering checkpoint on each passes or resume rendering with a previous checkpoint"));


write a rendering checkpoint after each pass, or resume rendering from the last checkpoint

src/appleseed/CMakeLists.txt

dictoon · 2018-08-29T13:15:59Z

src/appleseed/renderer/utility/filesystem.h

+// Boost headers.
+#include "boost/filesystem.hpp"
+
+namespace bf = boost::filesystem;


No namespace aliases in header files.

dictoon · 2018-08-29T13:17:13Z

src/appleseed/renderer/utility/bbox.h

@@ -56,6 +56,19 @@ BBox compute_union(const Iterator begin, const Iterator end);
 template <typename BBox, typename Iterator>
 BBox interpolate(const Iterator begin, const Iterator end, const double time);

+template <typename BBox, typename Tile, typename Int>
+BBox compute_tile_image_space_bbox(


Maybe compute_tile_bbox_image_space() would be clearer.

dictoon · 2018-08-29T13:19:43Z

src/appleseed/renderer/utility/bbox.h

+    const Int       tile_y)
+{
+    BBox tile_bbox;
+    const Int tile_origin_x = tile_x * static_cast<Int>(tile.get_width());


This is incorrect. A tile may have smaller dimensions than others because it's on the border of a frame. For instance a rightmost tile may have dimensions 50x64 while all tiles on its left have dimensions 64x64.

IIRC the box is then intersected with the frame. I think i tested that but I will check again.

dictoon · 2018-08-29T13:46:43Z

src/appleseed/renderer/modeling/frame/frame.cpp

+
+    // When resuming a render, first pass index should be
+    // the number of the resumed render's pass + 1.
+    // Otherwise it' 0.


dictoon · 2018-08-29T13:57:32Z

src/appleseed/renderer/modeling/aov/invalidsamplesaov.cpp

+        }
+
+      private:
+        size_t                                  m_invalid_sample_count;


We align data members among themselves, not necessarily with the unrelated surrounding stuff.

dictoon · 2018-08-29T13:59:02Z

src/appleseed/renderer/kernel/rendering/generic/genericframerenderer.h

+        const Frame&                        frame,
+        ITileRendererFactory*               tile_renderer_factory,
+        IShadingResultFrameBufferFactory*   framebuffer_factory,
+        ITileCallbackFactory*               tile_callback_factory,      // may be 0


Actually the comments here should say may be nullptr.

dictoon · 2018-08-29T14:00:40Z

src/appleseed/foundation/image/genericprogressiveimagefilereader.h

+        const size_t        tile_y,
+        Tile*               output_tile);
+
+    bool choose_subimage(const size_t subimage) const;


Comment. Move right above the two read_tile() methods.

dictoon · 2018-08-29T14:01:03Z

src/appleseed/foundation/image/genericprogressiveimagefilereader.cpp

@@ -139,6 +141,7 @@ struct GenericProgressiveImageFileReader::Impl
            static_cast<size_t>(spec.nchannels),
            pixel_format);
    }
+


est77 · 2018-08-30T08:59:55Z

src/appleseed/renderer/kernel/aov/aovaccumulator.h

-    UnfilteredAOVAccumulator(
-        foundation::Image& image,
-        foundation::Image& filter_image);
+    UnfilteredAOVAccumulator(foundation::Image& image);


Make constructor explicit now that it has only one argument

est77 · 2018-08-30T09:14:02Z

src/appleseed/renderer/modeling/aov/aov.cpp

@@ -110,12 +93,12 @@ ColorAOV::ColorAOV(const char* name, const ParamArray& params)

 size_t ColorAOV::get_channel_count() const
 {
-    return 3;
+    return 4;


I am not sure about this change. Most of our color AOVs are 3 channel.
Why did you change it?

Because the SadingBuffer is doing 4 * aov_count. So wether we change the shading buffer to use the exact amount of channels which can be difficult or we set all AOV to 4 channels if they are used in the shading buffer.

dictoon

Excellent code.

Made some remarks.

Thanks!

dictoon · 2018-12-19T12:20:17Z

src/appleseed/renderer/utility/filesystem.cpp

+}
+
+}       // namespace renderer
+


Remove redundant blank line. The file should simply end with a \n.

dictoon · 2018-12-19T12:21:34Z

src/appleseed/renderer/modeling/frame/frame.h

@@ -62,6 +62,7 @@ namespace foundation    { class Tile; }
 namespace renderer      { class BaseGroup; }
 namespace renderer      { class DenoiserAOV; }
 namespace renderer      { class ImageStack; }
+namespace renderer      { class PermanentShadingResultFrameBufferFactory; }


Order alphabetically.

dictoon · 2018-12-19T12:26:20Z

src/appleseed/renderer/kernel/rendering/renderercomponents.h

+    std::unique_ptr<ISampleRendererFactory>                         m_sample_renderer_factory;
+    std::unique_ptr<ISampleGeneratorFactory>                        m_sample_generator_factory;
+    std::unique_ptr<IPixelRendererFactory>                          m_pixel_renderer_factory;
+    foundation::auto_release_ptr<IShadingResultFrameBufferFactory>  m_shading_result_framebuffer_factory;


Why this change?

Because the ptr is not unique anymore.

The master renderer use it.

That's not a valid argument :) foundation::auto_release_ptr<> is like the now-deprecated std::auto_ptr<>, which is the ancestor of std::unique_ptr<>. All three are "unique pointers".

The only relevant difference between foundation::auto_release_ptr<> and std::unique_ptr<> is that the former calls a release() method to destroy the object while the later calls the destructor.

In this context, either points would work. I don't see any reason to change the type in this PR.

I didn't knew that :o The names aren't really helping.

dictoon · 2018-12-19T12:26:56Z

src/appleseed/renderer/kernel/rendering/masterrenderer.cpp

@@ -461,13 +462,24 @@ struct MasterRenderer::Impl
        // Updating the trace context causes ray tracing acceleration structures to be updated or rebuilt.
        m_project.update_trace_context();

+        // Load the checkpoint if any.
+        Frame& frame = *m_project.get_frame();
+


Remove this blank line and the one below so that the comment applies to the whole block.

dictoon · 2018-12-19T12:27:41Z

src/appleseed/renderer/kernel/rendering/masterrenderer.cpp

+        // Load the checkpoint if any.
+        Frame& frame = *m_project.get_frame();
+
+        PermanentShadingResultFrameBufferFactory* buffer_factory =


What guarantees that the shading result framebuffer is a permanent one here?

The code that generate the renderer and the buffer :D You think I should dynamic cast and test the result ?

dictoon · 2018-12-19T12:48:06Z

src/appleseed/renderer/modeling/frame/frame.cpp

+
+        // Save denoiser checkpoint.
+        if (denoiser_aov != nullptr)
+        {


Remove braces around single line statement.

dictoon · 2018-12-19T12:48:41Z

src/appleseed/renderer/modeling/frame/frame.cpp

+        }
+    }
+
+    RENDERER_LOG_INFO("wrote checkpoint for pass %s.", pretty_uint(pass + 1).c_str());


Let's mention the checkpoint file path.

wrote checkpoint file %s for pass %s.

dictoon · 2018-12-19T12:50:58Z

src/appleseed/renderer/modeling/frame/frame.cpp

+      public:
+        ShadingBufferCanvas(
+            const Frame&                                frame,
+            PermanentShadingResultFrameBufferFactory*   buffer_factory)


Do you really need a PermanentShadingResultFrameBufferFactory? What for?

Yes because EphemeralShadingResultFrameBufferFactory always create a new tile. But here I want the tiles that was already created and filled.

If the ShadingBufferCanvas would work with any IShadingResultFrameBufferFactory, then you wouldn't need casts or any special cases anywhere.

dictoon · 2018-12-19T12:51:11Z

src/appleseed/renderer/modeling/frame/frame.cpp

+            const CanvasProperties& props = image.properties();
+
+            // Compute the image space bounding box of the tile.
+            const int tile_origin_x = static_cast<int>(


Single line.

dictoon · 2018-12-19T12:51:15Z

src/appleseed/renderer/modeling/frame/frame.cpp

+            // Compute the image space bounding box of the tile.
+            const int tile_origin_x = static_cast<int>(
+                props.m_tile_width * tile_x);
+            const int tile_origin_y = static_cast<int>(


Single line.

oktomus · 2018-12-19T18:47:38Z

Thanks for the review @dictoon, really appreciated.

not up to date anymore

dictoon · 2019-01-16T08:23:12Z

src/appleseed/renderer/modeling/frame/frame.cpp

+    // Create channel names.
+    const size_t shading_channel_count = weight_canvas.properties().m_channel_count;
+    vector<string> shading_channel_names; // for shading buffer
+    vector<const char *> shading_channel_names_cr;


What does the _cr suffix mean here? What about _cstr?

It was for char. but _cstr is better indeed

dictoon · 2019-01-16T09:04:21Z

src/appleseed/renderer/modeling/frame/frame.cpp

+    writer.set_image_attributes(image_attributes);
+
+    writer.append_image(&weight_canvas);
+    image_attributes.insert("image_name", "appleseed:RenderingBuffer");


Reusing image_attributes is confusing and potentially dangerous: you might end up writing attributes you didn't intend to. For instance, right now the main image and all AOV images have a appleseed:LastPass attribute, is that intended?

If possible, I would suggest create a new ImageAttributes object every time. If necessary, you can wrap each block in braces ({}) in order to isolate them from each other.

Not it wasn't intended, thanks for pointing it out

dictoon · 2019-01-16T09:35:50Z

src/appleseed/renderer/modeling/frame/frame.cpp

+    writer.set_image_attributes(image_attributes);
+
+    // Add AOV layers.
+    for (size_t i = 0, e = aovs().size(); i < e; ++i)


Whenever possible, we should use range-based for loops:

for (const AOV& aov : aovs()) { ... }

dictoon · 2019-01-16T09:39:45Z

src/appleseed/renderer/modeling/frame/frame.cpp

+    {
+        const AOV* aov = internal_aovs().get_by_index(i);
+
+        const DenoiserAOV* denoiser_aov = dynamic_cast<const DenoiserAOV*>(aov);


This line belongs to the block below:

// Save denoiser checkpoint. const DenoiserAOV* denoiser_aov = dynamic_cast<const DenoiserAOV*>(aov); if (denoiser_aov != nullptr) save_denoiser_checkpoint(impl->m_checkpoint_create_path, denoiser_aov);

src/appleseed/renderer/modeling/frame/frame.cpp

dictoon · 2019-01-16T09:48:05Z

src/appleseed/renderer/kernel/rendering/renderercomponents.h

+    std::unique_ptr<ISampleRendererFactory>                         m_sample_renderer_factory;
+    std::unique_ptr<ISampleGeneratorFactory>                        m_sample_generator_factory;
+    std::unique_ptr<IPixelRendererFactory>                          m_pixel_renderer_factory;
+    foundation::auto_release_ptr<IShadingResultFrameBufferFactory>  m_shading_result_framebuffer_factory;


That's not a valid argument :) foundation::auto_release_ptr<> is like the now-deprecated std::auto_ptr<>, which is the ancestor of std::unique_ptr<>. All three are "unique pointers".

The only relevant difference between foundation::auto_release_ptr<> and std::unique_ptr<> is that the former calls a release() method to destroy the object while the later calls the destructor.

In this context, either points would work. I don't see any reason to change the type in this PR.

dictoon · 2019-01-16T09:53:11Z

src/appleseed/renderer/modeling/frame/frame.cpp

+      public:
+        ShadingBufferCanvas(
+            const Frame&                                frame,
+            PermanentShadingResultFrameBufferFactory*   buffer_factory)


If the ShadingBufferCanvas would work with any IShadingResultFrameBufferFactory, then you wouldn't need casts or any special cases anywhere.

dictoon · 2019-01-16T09:53:53Z

src/appleseed/renderer/kernel/rendering/generic/genericframerenderer.cpp

@@ -316,11 +323,17 @@ namespace
            {
                set_current_thread_name("pass_manager");

+                const size_t start_pass = m_frame.get_initial_pass();
+
+                PermanentShadingResultFrameBufferFactory* buffer_factory =


See my other comment in frame.cpp. We want to avoid those potentially dangerous casts.

It will work without casting but the buffer factory needs to be permanent. Can I point that out in a comment and forget about the cast ?

Why does it need to be permanent? What happens if it's not (and assuming you don't cast anymore)?

If it's not permanent, it means it's ephemeral and it will create an empty tile. So there no point in saving this.

I understand, but wouldn't be a correct behavior? If an ephemeral buffer is used, then you get empty checkpoints, no problem, no special case. It's kind of the same as saying, if there are no lights in the scene, the render will be black, so we should abort instead of rendering. We don't, there's no special case for a scene without any light.

dictoon · 2019-01-16T09:55:45Z

src/appleseed/renderer/modeling/frame/frame.cpp

+                m_frame.image().properties().m_canvas_height,
+                m_frame.image().properties().m_tile_width,
+                m_frame.image().properties().m_tile_height,
+                (1 + m_frame.aov_images().size()) * 4 + 1,


Where does that expression come from? Is it the responsability of ShadingBufferCanvas to compute that number of channels, are you trying to match the number of channels computed elsewhere?

Indeed I'm trying to match the shading buffer number of channels.
https://github.com/appleseedhq/appleseed/blob/master/src/appleseed/renderer/kernel/rendering/shadingresultframebuffer.cpp#L53

Why the additional + 1?
Can you instead query the number of channels in the ShadingResultFrameBuffer instead?

+1 Because i save the beauty image, just in case you want to correctly visualize the checkpoint

- Expose a checkpoint option in cli - Create a checkpoint file after each pass if "--checkpoint" is on - Restore from a checkpoint file if "--checkpoint" is on - Handle denoiser when saving and restoring a checkpoint file (informations are not saved in the same checkpoint file) - Fix passes option path in cli and studio - Minor changes

dictoon

Looks a lot better!

It's almost ready for merging. The few remaining comments are all small details and it should only take a few minutes to address them. Once these are addressed, we can (finally) merge this PR.

Thanks for the excellent work!

src/appleseed/renderer/modeling/project/configuration.cpp

dictoon · 2019-02-12T21:43:52Z

src/appleseed/renderer/modeling/frame/frame.h

+        foundation::IAbortSwitch*                   abort_switch) const;
+
+    // Open the checkpoint if checkpoint is enabled and the file located
+    // at checkpoint_path exists and is valid.


Is the comment up-to-date here? What's checkpoint_path?

dictoon · 2019-02-12T21:45:27Z

src/appleseed/renderer/kernel/rendering/shadingresultframebuffer.h

@@ -80,9 +80,17 @@ class ShadingResultFrameBuffer
        foundation::Tile&               tile,
        TileStack&                      aov_tiles) const;

+    static inline size_t get_total_channel_count(const size_t aov_count);


Remove inline here.

Move static method before non-static ones.

dictoon · 2019-02-12T21:45:32Z

src/appleseed/renderer/kernel/rendering/shadingresultframebuffer.h

  private:
    const size_t                        m_aov_count;
    std::vector<float>                  m_scratch;
 };

+size_t ShadingResultFrameBuffer::get_total_channel_count(const size_t aov_count)


Add inline here.

dictoon · 2019-02-12T21:47:52Z

src/appleseed/renderer/kernel/rendering/masterrenderer.cpp

@@ -485,6 +490,7 @@ struct MasterRenderer::Impl
        recorder.on_render_end(m_project);

        const CanvasProperties& props = m_project.get_frame()->image().properties();
+


dictoon · 2019-02-12T22:02:46Z

src/appleseed/renderer/modeling/frame/frame.cpp

-        image_attributes.insert("red_xy_chromaticity", Vector2f(0.64f, 0.33f));
-        image_attributes.insert("green_xy_chromaticity", Vector2f(0.30f, 0.60f));
-        image_attributes.insert("blue_xy_chromaticity",  Vector2f(0.15f, 0.06f));
+        // The beauty image plus the shadding result framebuffer.


dictoon · 2019-02-12T22:08:29Z

src/appleseed/renderer/modeling/frame/frame.cpp

+            if (extension != ".exr")
+            {
+                // Only .exr files are allowed.
+                RENDERER_LOG_ERROR("checkpoint file must be an \".exr\" file, disabling checkpoint creation");


End message with a period (.).

dictoon · 2019-02-12T22:08:33Z

src/appleseed/renderer/modeling/frame/frame.cpp

+            if (extension != ".exr")
+            {
+                // Only .exr files are allowed.
+                RENDERER_LOG_ERROR("checkpoint file must be an \".exr\" file, disabling checkpoint resuming");


End message with a period (.).

dictoon · 2019-02-12T22:08:58Z

src/appleseed/renderer/modeling/frame/frame.cpp

+        {
+            string path = m_params.get_optional<string>("checkpoint_create_path", "");
+            const bool use_default_path = path.empty();
+            if (use_default_path) path = m_params.get_required<string>("output_path");


Move if body to separate line.

dictoon · 2019-02-12T22:09:02Z

src/appleseed/renderer/modeling/frame/frame.cpp

+        {
+            string path = m_params.get_optional<string>("checkpoint_resume_path", "");
+            const bool use_default_path = path == "";
+            if (use_default_path) path = m_params.get_required<string>("output_path");


Move if body to separate line.

dictoon

Thanks!

oktomus added the Component | appleseed.cli label Jul 12, 2018

dictoon added the PR | Squash This PR must be squashed when merged. label Jul 18, 2018

oktomus force-pushed the cli-checkpoint-renders branch 11 times, most recently from 4752d68 to 355cdd3 Compare July 31, 2018 08:03

oktomus force-pushed the cli-checkpoint-renders branch 7 times, most recently from 8b58fa1 to b145095 Compare August 1, 2018 12:13

oktomus added PR | Rebase This PR must be rebased when merged. and removed PR | Squash This PR must be squashed when merged. labels Aug 1, 2018

oktomus force-pushed the cli-checkpoint-renders branch from b145095 to ddcc35b Compare August 1, 2018 12:27

oktomus changed the title ~~[WIP] Expose checkpoint option and save/restore renders in cli~~ Expose checkpoint option and save/restore renders in cli Aug 1, 2018

oktomus force-pushed the cli-checkpoint-renders branch 4 times, most recently from 89a342f to b9a6b15 Compare August 2, 2018 14:25

dictoon requested changes Aug 29, 2018

View reviewed changes

est77 previously requested changes Aug 30, 2018

View reviewed changes

est77 reviewed Aug 30, 2018

View reviewed changes

oktomus force-pushed the cli-checkpoint-renders branch 2 times, most recently from 49055f4 to 691a514 Compare September 16, 2018 10:33

dictoon added the On Hold 🔶 label Oct 21, 2018

dictoon removed the On Hold 🔶 label Nov 9, 2018

oktomus added the WIP ⚠ label Nov 16, 2018

oktomus force-pushed the cli-checkpoint-renders branch from 691a514 to 5156950 Compare December 4, 2018 18:08

oktomus removed the WIP ⚠ label Dec 4, 2018

oktomus force-pushed the cli-checkpoint-renders branch 2 times, most recently from 8198a19 to 4cdb4fd Compare December 11, 2018 18:21

dictoon requested changes Dec 19, 2018

View reviewed changes

oktomus force-pushed the cli-checkpoint-renders branch from b306f72 to 2705c01 Compare December 30, 2018 13:11

oktomus force-pushed the cli-checkpoint-renders branch from 2705c01 to 8bc8cba Compare January 11, 2019 08:46

dictoon reviewed Jan 16, 2019

View reviewed changes

dictoon requested changes Jan 16, 2019

View reviewed changes

oktomus added 5 commits February 12, 2019 19:38

review 01: cosmetics

3e09fea

review 02: refactor read_tile in progressive file reader

f367d61

review 03: use actual options names

8cfcabb

review 04: pointers, cleaning and stuff

4a1d191

oktomus force-pushed the cli-checkpoint-renders branch from 8bc8cba to 4a1d191 Compare February 12, 2019 18:49

dictoon reviewed Feb 12, 2019

View reviewed changes

review 05: cosmetics

b8ac4d3

dictoon approved these changes Feb 14, 2019

View reviewed changes

dictoon added PR | Squash This PR must be squashed when merged. and removed PR | Rebase This PR must be rebased when merged. labels Feb 14, 2019

dictoon merged commit a9a25a8 into appleseedhq:master Feb 14, 2019

		@@ -485,6 +490,7 @@ struct MasterRenderer::Impl
		recorder.on_render_end(m_project);

		const CanvasProperties& props = m_project.get_frame()->image().properties();

Expose checkpoint option and save/restore renders in cli #2120

Expose checkpoint option and save/restore renders in cli #2120

Conversation

oktomus commented Jul 12, 2018 • edited Loading

Future Work

dictoon commented Aug 29, 2018

dictoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dictoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oktomus commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dictoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dictoon left a comment

Choose a reason for hiding this comment

oktomus commented Jul 12, 2018 •

edited

Loading