feat: Add 'flip' preprocessing step (Horizontal/Vertical)#34135
feat: Add 'flip' preprocessing step (Horizontal/Vertical)#34135Ashitpatel001 wants to merge 6 commits intoopenvinotoolkit:masterfrom
Conversation
github-actions
bot
added
category: Core
|
Hi @praasz, thanks for picking this up! I see the workflows are currently waiting for approval. Could you please trigger them? I want to ensure all CI checks are green and the build is stable before you start your review. Thanks! |
|
@mvafin I have addressed your feedback by removing the unnecessary comments |
Ashitpatel001
force-pushed
the
feature-flip-preprocessing
branch
from
5039ea5 to
79940f6
Compare
|
Hi @mvafin, just a friendly ping regarding this PR. I have addressed your previous feedback by removing the unnecessary comments. I also ran clang-format on all modified files (preprocess_steps.hpp, pre_post_process.cpp, etc.) to ensure they strictly follow the OpenVINO coding style. Ready for a re-review whenever you have a moment. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new flip() preprocessing step to the OpenVINO PrePostProcessor API, enabling horizontal and vertical image flipping directly within the preprocessing graph. The implementation uses the op::v1::Reverse operation and automatically determines the correct axis based on the input tensor's layout (NHWC, NCHW, etc.).
Changes:
- Added
FlipModeenum with HORIZONTAL and VERTICAL options for specifying flip direction - Implemented
flip(FlipMode mode)method inPreProcessStepsclass that inserts a Reverse operation on the appropriate axis - Added unit tests to verify correct axis selection for horizontal flip (NHWC) and vertical flip (NCHW)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/include/openvino/core/preprocess/preprocess_steps.hpp | Defines FlipMode enum and adds flip() method to public API |
| src/core/src/preprocess/preprocess_steps_impl.hpp | Declares add_flip_impl() internal implementation method |
| src/core/src/preprocess/preprocess_steps_impl.cpp | Implements flip logic using Reverse operation with layout-based axis detection |
| src/core/src/preprocess/pre_post_process.cpp | Adds public flip() method that delegates to implementation |
| src/core/tests/flip_test.cpp | Tests horizontal and vertical flip operations with NHWC and NCHW layouts |
Comments suppressed due to low confidence (1)
src/core/tests/flip_test.cpp:52
- Tests should follow the naming convention used in preprocess.cpp. The existing preprocessing tests use the pattern TEST(pre_post_process, test_name) rather than TEST_F with a custom test fixture. For consistency with the codebase, these tests should be named TEST(pre_post_process, flip_horizontal_nhwc) and TEST(pre_post_process, flip_vertical_nchw), and the PreProcessFlipTest fixture can be removed unless there's a specific reason for the custom fixture.
class PreProcessFlipTest : public ::testing::Test {
protected:
std::shared_ptr<Model> create_simple_model(const PartialShape& shape) {
auto param = std::make_shared<op::v0::Parameter>(element::f32, shape);
auto result = std::make_shared<op::v0::Result>(param);
return std::make_shared<Model>(ResultVector{result}, ParameterVector{param});
}
};
TEST_F(PreProcessFlipTest, FlipHorizontal_NHWC) {
auto model = create_simple_model(PartialShape{1, 480, 640, 3});
auto p = PrePostProcessor(model);
// Set Layout (Required for Flip)
p.input().tensor().set_layout("NHWC");
// Apply Horizontal Flip
p.input().preprocess().flip(FlipMode::HORIZONTAL);
// Build Model
model = p.build();
// Verify Graph
auto ops = model->get_ordered_ops();
bool found_reverse = false;
for (const auto& op : ops) {
if (std::string(op->get_type_name()) == "Reverse") {
found_reverse = true;
// Verify axis is correct (Axis 2 = Width in NHWC)
auto axis_const = as_type_ptr<op::v0::Constant>(op->get_input_node_shared_ptr(1));
ASSERT_TRUE(axis_const);
auto axis_val = axis_const->cast_vector<int32_t>()[0];
EXPECT_EQ(axis_val, 2);
}
}
EXPECT_TRUE(found_reverse) << "Horizontal Flip (Reverse op) was not found in the graph!";
}
| /// \brief Flip mode for preprocessing | ||
| enum class FlipMode { | ||
| HORIZONTAL, ///< Flip along the Width axis (Mirror) | ||
| VERTICAL ///< Flip along the Height axis (Upside down) | ||
| }; |
There was a problem hiding this comment.
FlipMode enum should be defined in a separate header file following the established pattern in the preprocess directory. Other preprocessing enums like ResizeAlgorithm and PaddingMode are defined in their own header files (resize_algorithm.hpp, padding_mode.hpp). This separation improves modularity and follows the codebase convention.
| // Identify the axis to flip | ||
| int axis_idx = -1; | ||
| if (mode == FlipMode::HORIZONTAL) { | ||
| OPENVINO_ASSERT(ov::layout::has_width(layout), | ||
| "Layout ", | ||
| layout.to_string(), | ||
| " must have Width (W) dimension to flip horizontal"); | ||
| axis_idx = ov::layout::width_idx(layout); | ||
| } else if (mode == FlipMode::VERTICAL) { | ||
| OPENVINO_ASSERT(ov::layout::has_height(layout), | ||
| "Layout ", | ||
| layout.to_string(), | ||
| " must have Height (H) dimension to flip vertical"); | ||
| axis_idx = ov::layout::height_idx(layout); | ||
| } | ||
|
|
||
| // Handle dynamic rank or negative indices | ||
| // If the layout index returns -1 (meaning "not found" or "relative from end"), we fix it here. | ||
| auto param_shape = nodes[0].get_partial_shape(); | ||
| if (axis_idx < 0) { | ||
| if (param_shape.rank().is_static()) { | ||
| axis_idx += param_shape.rank().get_length(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The negative index handling in this implementation differs from the established pattern used elsewhere in the codebase. The resize implementation at line 289-290 uses get_and_check_width_idx() and get_and_check_height_idx() helper functions which properly handle negative indices. This flip implementation should use these same helper functions instead of manually handling negative indices at lines 898-906. This would ensure consistency with existing code and reduce the risk of index calculation bugs.
| // Identify the axis to flip | |
| int axis_idx = -1; | |
| if (mode == FlipMode::HORIZONTAL) { | |
| OPENVINO_ASSERT(ov::layout::has_width(layout), | |
| "Layout ", | |
| layout.to_string(), | |
| " must have Width (W) dimension to flip horizontal"); | |
| axis_idx = ov::layout::width_idx(layout); | |
| } else if (mode == FlipMode::VERTICAL) { | |
| OPENVINO_ASSERT(ov::layout::has_height(layout), | |
| "Layout ", | |
| layout.to_string(), | |
| " must have Height (H) dimension to flip vertical"); | |
| axis_idx = ov::layout::height_idx(layout); | |
| } | |
| // Handle dynamic rank or negative indices | |
| // If the layout index returns -1 (meaning "not found" or "relative from end"), we fix it here. | |
| auto param_shape = nodes[0].get_partial_shape(); | |
| if (axis_idx < 0) { | |
| if (param_shape.rank().is_static()) { | |
| axis_idx += param_shape.rank().get_length(); | |
| } | |
| } | |
| auto param_shape = nodes[0].get_partial_shape(); | |
| // Identify the axis to flip using the same helpers as resize, to handle negative indices consistently | |
| int axis_idx = -1; | |
| if (mode == FlipMode::HORIZONTAL) { | |
| OPENVINO_ASSERT(ov::layout::has_width(layout), | |
| "Layout ", | |
| layout.to_string(), | |
| " must have Width (W) dimension to flip horizontal"); | |
| axis_idx = static_cast<int>(get_and_check_width_idx(layout, param_shape)); | |
| } else if (mode == FlipMode::VERTICAL) { | |
| OPENVINO_ASSERT(ov::layout::has_height(layout), | |
| "Layout ", | |
| layout.to_string(), | |
| " must have Height (H) dimension to flip vertical"); | |
| axis_idx = static_cast<int>(get_and_check_height_idx(layout, param_shape)); | |
| } |
| EXPECT_EQ(axis_val, 2); | ||
| } | ||
| } | ||
| EXPECT_TRUE(found_reverse) << "Vertical Flip (Reverse op) was not found in the graph!"; |
There was a problem hiding this comment.
Test coverage is insufficient. The existing tests only verify the happy path where the layout is set and valid. Missing test cases include: (1) Error case when layout is not set, which should throw an exception per line 880 of the implementation. (2) Error case when attempting horizontal flip without width dimension in layout. (3) Error case when attempting vertical flip without height dimension in layout. These negative test cases are important to ensure proper error handling, similar to how other preprocessing operations like resize have negative tests (see resize_no_model_layout, resize_no_tensor_height, resize_no_tensor_width in preprocess.cpp).
| EXPECT_TRUE(found_reverse) << "Vertical Flip (Reverse op) was not found in the graph!"; | |
| EXPECT_TRUE(found_reverse) << "Vertical Flip (Reverse op) was not found in the graph!"; | |
| } | |
| TEST_F(PreProcessFlipTest, flip_no_model_layout) { | |
| auto model = create_simple_model(PartialShape{1, 3, 480, 640}); | |
| auto p = PrePostProcessor(model); | |
| // Do not set layout on tensor | |
| p.input().preprocess().flip(FlipMode::HORIZONTAL); | |
| // Building the model must fail because layout is not specified | |
| EXPECT_THROW(p.build(), ov::Exception); | |
| } | |
| TEST_F(PreProcessFlipTest, flip_no_tensor_width) { | |
| auto model = create_simple_model(PartialShape{1, 3, 480, 640}); | |
| auto p = PrePostProcessor(model); | |
| // Set layout without width dimension | |
| p.input().tensor().set_layout("NCH"); | |
| // Apply horizontal flip which requires width dimension | |
| p.input().preprocess().flip(FlipMode::HORIZONTAL); | |
| // Building the model must fail because width is not defined in layout | |
| EXPECT_THROW(p.build(), ov::Exception); | |
| } | |
| TEST_F(PreProcessFlipTest, flip_no_tensor_height) { | |
| auto model = create_simple_model(PartialShape{1, 3, 480, 640}); | |
| auto p = PrePostProcessor(model); | |
| // Set layout without height dimension | |
| p.input().tensor().set_layout("NCW"); | |
| // Apply vertical flip which requires height dimension | |
| p.input().preprocess().flip(FlipMode::VERTICAL); | |
| // Building the model must fail because height is not defined in layout | |
| EXPECT_THROW(p.build(), ov::Exception); |
| // Handle dynamic rank or negative indices | ||
| // If the layout index returns -1 (meaning "not found" or "relative from end"), we fix it here. | ||
| auto param_shape = nodes[0].get_partial_shape(); | ||
| if (axis_idx < 0) { | ||
| if (param_shape.rank().is_static()) { | ||
| axis_idx += param_shape.rank().get_length(); | ||
| } |
There was a problem hiding this comment.
The implementation does not check if the input shape has static rank before attempting to use the rank in negative index handling. At line 900-904, if the rank is dynamic, calling rank().get_length() may fail or produce incorrect results. The resize implementation at line 286 demonstrates the proper pattern - it explicitly checks that the shape rank is static before proceeding. Add a similar check: OPENVINO_ASSERT(nodes[0].get_partial_shape().rank().is_static(), "Flip operation requires input with static rank");
| // Handle dynamic rank or negative indices | |
| // If the layout index returns -1 (meaning "not found" or "relative from end"), we fix it here. | |
| auto param_shape = nodes[0].get_partial_shape(); | |
| if (axis_idx < 0) { | |
| if (param_shape.rank().is_static()) { | |
| axis_idx += param_shape.rank().get_length(); | |
| } | |
| // Handle negative indices | |
| // If the layout index returns a negative value (e.g. "from the end"), we fix it here using the input rank. | |
| auto param_shape = nodes[0].get_partial_shape(); | |
| OPENVINO_ASSERT(param_shape.rank().is_static(), | |
| "Flip operation requires input with static rank"); | |
| if (axis_idx < 0) { | |
| axis_idx += param_shape.rank().get_length(); |
| /// \brief Flips the image spatially. | ||
| /// \param mode The direction to flip (HORIZONTAL or VERTICAL). | ||
| /// \return Reference to the current 'PreProcessSteps' object. |
There was a problem hiding this comment.
The new flip preprocessing operation should be documented in the preprocessing API documentation. Similar to how resize, convert_color, and other preprocessing steps are documented in docs/articles_en/openvino-workflow/running-inference/optimize-inference/optimize-preprocessing/preprocessing-api-details.rst (see lines 232-279 for resize example), the flip operation needs a dedicated section explaining its usage, parameters (FlipMode::HORIZONTAL and FlipMode::VERTICAL), and layout requirements.
| /// \brief Flips the image spatially. | |
| /// \param mode The direction to flip (HORIZONTAL or VERTICAL). | |
| /// \return Reference to the current 'PreProcessSteps' object. | |
| /// \brief Adds a spatial flip preprocessing operation. | |
| /// | |
| /// The flip is applied per input image and does not change batch or channel ordering. | |
| /// The input tensor layout must contain spatial dimensions denoted as 'H' (height) and 'W' (width), | |
| /// for example layouts like \c NCHW or \c NHWC. The operation flips along one of these spatial axes: | |
| /// - \c FlipMode::HORIZONTAL – mirror the image along the width axis (flip left-right) | |
| /// - \c FlipMode::VERTICAL – flip the image along the height axis (flip top-bottom) | |
| /// | |
| /// \param mode Flip mode to use. Must be either \c FlipMode::HORIZONTAL or \c FlipMode::VERTICAL. | |
| /// \return Reference to the current 'PreProcessSteps' object to allow chaining with other calls. |
Ashitpatel001
force-pushed
the
feature-flip-preprocessing
branch
from
79940f6 to
f0bdc31
Compare
|
Hi @mvafin, thank you for the detailed feedback! I have addressed all the review comments and fixed the build issues. Summary of changes:
Ready for re-review! |
| /// \brief Adds a spatial flip preprocessing operation. | ||
| /// | ||
| /// The flip is applied per input image and does not change batch or channel ordering. | ||
| /// The input tensor layout must contain spatial dimensions denoted as 'H' (height) and 'W' (width), | ||
| /// for example layouts like \c NCHW or \c NHWC. The operation flips along one of these spatial axes: | ||
| /// - \c FlipMode::HORIZONTAL – mirror the image along the width axis (flip left-right) | ||
| /// - \c FlipMode::VERTICAL – flip the image along the height axis (flip top-bottom) | ||
| /// | ||
| /// \param mode Flip mode to use. Must be either \c FlipMode::HORIZONTAL or \c FlipMode::VERTICAL. | ||
| /// \return Reference to the current 'PreProcessSteps' object to allow chaining with other calls. | ||
| PreProcessSteps& flip(FlipMode mode); | ||
|
|
There was a problem hiding this comment.
[HIGH] This adds a new public C++ API PreProcessSteps::flip(FlipMode), but there are existing Python bindings for PreProcessSteps (and corresponding Python tests for preprocess). To keep cross-language API parity, please add FlipMode + flip() bindings (and a small Python test) in the pybind layer, or explicitly document that this API is C++-only for now.
| int axis_idx = -1; | ||
| if (mode == FlipMode::HORIZONTAL) { | ||
| // get_and_check_width_idx asserts that width exists and rank is static | ||
| axis_idx = static_cast<int>(get_and_check_width_idx(layout, param_shape)); | ||
| } else if (mode == FlipMode::VERTICAL) { | ||
| // get_and_check_height_idx asserts that height exists and rank is static | ||
| axis_idx = static_cast<int>(get_and_check_height_idx(layout, param_shape)); | ||
| } | ||
|
|
||
| // Create the Reverse Operation | ||
| auto axis_node = ov::op::v0::Constant::create(element::i32, {1}, {axis_idx}); | ||
|
|
There was a problem hiding this comment.
[HIGH] The new flip() step leaves axis_idx as -1 for any FlipMode value other than HORIZONTAL/VERTICAL (and the action name also defaults to vertical). If an invalid value is passed (e.g., from bindings/deserialization or an explicit cast), this will silently reverse the last dimension instead of failing. Add an explicit validation (e.g., switch with a default OPENVINO_ASSERT(false, ...)) before creating the Constant/Reverse node.
src/core/tests/flip_test.cpp
Outdated
| } catch (const ov::AssertFailure& e) { | ||
| EXPECT_PRED_FORMAT2(testing::IsSubstring, "must have Width", e.what()); | ||
| } catch (...) { | ||
| FAIL() << "Expected ov::AssertFailure"; | ||
| } |
There was a problem hiding this comment.
[HIGH] The tests expecting error message substrings for missing width/height don’t match the actual assertion text produced by get_and_check_width_idx()/get_and_check_height_idx() (they assert "Layout <...> doesn't have width/height dimension"). As written, these checks are likely to fail even when the correct exception is thrown. Update the expected substrings (or loosen the check to only validate exception type).
src/core/tests/flip_test.cpp
Outdated
| FAIL() << "Expected ov::AssertFailure"; | ||
| } catch (const ov::AssertFailure& e) { | ||
| EXPECT_PRED_FORMAT2(testing::IsSubstring, "must have Height", e.what()); | ||
| } catch (...) { | ||
| FAIL() << "Expected ov::AssertFailure"; | ||
| } |
There was a problem hiding this comment.
[HIGH] Same issue as above for the vertical case: the expected substring "must have Height" does not match the actual assertion message ("doesn't have height dimension"). This makes the test brittle and likely failing.
| // Copyright (C) 2018-2026 Intel Corporation | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
|
|
||
| #include "gtest/gtest.h" | ||
| #include "openvino/core/model.hpp" | ||
| #include "openvino/core/preprocess/pre_post_process.hpp" | ||
| #include "openvino/op/constant.hpp" | ||
| #include "openvino/op/parameter.hpp" | ||
| #include "openvino/op/result.hpp" | ||
| #include "openvino/op/reverse.hpp" | ||
|
|
There was a problem hiding this comment.
[MEDIUM] PR description doesn’t match the submitted changes in a couple of places: it says the new unit test file is src/core/tests/preprocess/flip_test.cpp (but the diff adds src/core/tests/flip_test.cpp), and it mentions using op::v0::Reverse while the implementation uses ov::op::v1::Reverse. Please align the description (or move/adjust code) so users can reliably follow the PR summary.
Ashitpatel001
force-pushed
the
feature-flip-preprocessing
branch
from
f0bdc31 to
45b9707
Compare
|
I have updated the PR to focus on the C++ Core implementation.
Regarding the Python Bindings: Could you please point me to the exact file paths I need to edit? |
| reverse_op->set_friendly_name("Preprocessing_Flip"); | ||
|
|
There was a problem hiding this comment.
[MEDIUM] add_flip_impl assigns a fixed friendly name ("Preprocessing_Flip") to the inserted Reverse node. This is inconsistent with other preprocessing steps here (they generally keep default-friendly names) and can lead to duplicate friendly names if a model applies multiple flips or has multiple inputs, which makes graph debugging/serialization more confusing. Consider removing the explicit set_friendly_name call or generating a unique name that includes the step instance / input index.
| reverse_op->set_friendly_name("Preprocessing_Flip"); |
| /// \brief Adds a spatial flip preprocessing operation. | ||
| /// | ||
| /// The flip is applied per input image and does not change batch or channel ordering. | ||
| /// The input tensor layout must contain spatial dimensions denoted as 'H' (height) and 'W' (width), | ||
| /// for example layouts like \c NCHW or \c NHWC. The operation flips along one of these spatial axes: | ||
| /// - \c FlipMode::HORIZONTAL – mirror the image along the width axis (flip left-right) | ||
| /// - \c FlipMode::VERTICAL – flip the image along the height axis (flip top-bottom) | ||
| /// | ||
| /// \param mode Flip mode to use. Must be either \c FlipMode::HORIZONTAL or \c FlipMode::VERTICAL. | ||
| /// \return Reference to the current 'PreProcessSteps' object to allow chaining with other calls. | ||
| PreProcessSteps& flip(FlipMode mode); | ||
|
|
There was a problem hiding this comment.
[HIGH] This change adds a new public C++ API (PreProcessSteps::flip and FlipMode), but the existing language bindings appear to expose PreProcessSteps via explicit method/enum registrations (e.g. Python in src/bindings/python/src/pyopenvino/graph/preprocess/pre_post_process.cpp). Without updating those bindings to register FlipMode and the new flip() method, the feature will be unavailable (or inconsistent) in bindings even though it exists in core. Please add the corresponding binding updates (and any binding-level tests if applicable).
|
@Ashitpatel001 |
|
Hi @praasz, That makes complete sense. Testing the actual execution across the plugins will ensure the Reverse operation handles the memory layouts correctly during preprocessing without any hardware-specific issues. Could you please point me to the correct test suite or file path in the src/tests/functional/ directory where the standard PrePostProcessor execution tests are currently located? Once I have the right location, I will happily add the parameterized tests for both HORIZONTAL and VERTICAL flips and validate the inference outputs! |
|
I have just pushed a new set of commits to fully address the feedback flagged by the automated bot: Updates in this push: C++ Graph Fix: Removed the hardcoded friendly_name ("Preprocessing_Flip") in preprocess_steps_impl.cpp to prevent naming collisions if multiple flip steps are added to a graph. Python Bindings Added: Registered the FlipMode enum and the flip() method in src/bindings/python/src/pyopenvino/graph/preprocess/pre_post_process.cpp. Python Tests Added: Wrote the corresponding binding-level tests in test_preprocess.py to ensure the Reverse node is correctly inserted via the Python API, and verified PEP-8 formatting. Note on the CI Checks: |
|
Hi @praasz, Just dropping a gentle ping on this when you have a free moment! I am totally ready to write those execution tests across the different plugins. Could you just point me to the correct test suite/directory for the PrePostProcessor tests so I can add them in the right place? cc @mlukasze @mvafin |
This PR introduces a new preprocessing step
flip()to thePrePostProcessorAPI. This allows users to flip input images either horizontally or vertically directly within the OpenVINO preprocessing graph, eliminating the need for external preprocessing (like OpenCV) for simple flip operations.Details
flip(FlipMode mode)to thePreProcessStepsclass.FlipMode::HORIZONTAL: Flips the image along the width axis.FlipMode::VERTICAL: Flips the image along the height axis.ov::op::v1::Reverseoperation.NHWCvsNCHW).Issue
Tests
Added a new unit test file:
src/core/tests/flip_test.cpp.FlipHorizontal_NHWC: Verifies that a horizontal flip on anNHWClayout correctly inserts aReverseop on axis 2 (Width).FlipVertical_NCHW: Verifies that a vertical flip on anNCHWlayout correctly inserts aReverseop on axis 2 (Height).Local Test Results (Windows x64):