ronantakizawa/codereview-bench huggingface dataset:
Repository: opencv/opencv
File: modules/gapi/include/opencv2/gapi/infer/ov.hpp (C++)

Code before:
};

} // namespace ov

namespace wip { namespace ov {
/**
 * @brief Ask G-API OpenVINO backend to run only inference of model provided.
 *
 * G-API OpenVINO backend will perform only the inference of the model provided
 * without populating input and copying back output data.
 * This mode is used to evaluate the pure inference performance of the model without
 * taking into account the i/o data transfer.
 */
struct benchmark_mode { };
struct workload_type {
    using callback = std::function<void(const std::string &type)>;
    using listener = std::pair<unsigned int, callback>;
    unsigned int addListener(callback cb){
        unsigned int id = nextId++;
        listeners.emplace_back(id, std::move(cb));
        return id;
    }
    void removeListener(unsigned int id) {
        listeners.erase(std::remove_if(listeners.begin(), listeners.end(), [=](listener& pair){return pair.first == id;}), listeners.end());
    }
    void setWorkloadType(const std::string &type) {
        for(const listener &l : listeners) {
            l.second(type);
        }
    }
 private:
    std::vector<listener> listeners;
    unsigned int nextId = 0;
};
    using WorkloadTypeRef = std::reference_wrapper<cv::gapi::wip::ov::workload_type>;

// #endif
} // namespace ov
} // namespace wip

} // namespace gapi

namespace detail
{
    template<> struct CompileArgTag<cv::gapi::wip::ov::benchmark_mode>
    {
        static const char* tag() { return "gapi.wip.ov.benchmark_mode"; }
    };
    template<> struct CompileArgTag<std::reference_wrapper<cv::gapi::wip::ov::workload_type>>
    {
        static const char* tag() { return "gapi.wip.ov.workload_type_ref"; }

Review comment (suggestion):
I think it is a bad name for the method, as our struct has already `workload_type` name. Might be just `set` will work or copy-assignment, move-assignment operators and constructors.

Code after:
    detail::ParamDesc m_desc;
};

} // namespace ov

namespace wip { namespace ov {
/**
 * @brief Ask G-API OpenVINO backend to run only inference of model provided.
 *
 * G-API OpenVINO backend will perform only the inference of the model provided
 * without populating input and copying back output data.
 * This mode is used to evaluate the pure inference performance of the model without
 * taking into account the i/o data transfer.
 */
struct benchmark_mode { };

class WorkloadTypeOV : public WorkloadType {};
using WorkloadTypeOVPtr = std::shared_ptr<cv::gapi::wip::ov::WorkloadTypeOV>;

} // namespace ov
} // namespace wip

} // namespace gapi

namespace detail
{
    template<> struct CompileArgTag<cv::gapi::wip::ov::benchmark_mode>
    {
        static const char* tag() { return "gapi.wip.ov.benchmark_mode"; }
    };
    template<> struct CompileArgTag<cv::gapi::wip::ov::WorkloadTypeOVPtr>
    {
        static const char* tag() { return "gapi.wip.ov.workload_type"; }
    };
}

} // namespace cv

#endif // OPENCV_GAPI_INFER_OV_HPP
Repository: opencv/opencv
File: modules/gapi/include/opencv2/gapi/infer/onnx.hpp (C++)

Code before:
// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.
//
// Copyright (C) 2020-2021 Intel Corporation

#ifndef OPENCV_GAPI_INFER_ONNX_HPP
#define OPENCV_GAPI_INFER_ONNX_HPP

#include <unordered_map>
#include <string>
#include <array>
#include <tuple> // tuple, tuple_size
#include <map>

#include <opencv2/gapi/opencv_includes.hpp>
#include <opencv2/gapi/util/any.hpp>
#include <opencv2/gapi/util/optional.hpp>

#include <opencv2/core/cvdef.h>     // GAPI_EXPORTS
#include <opencv2/gapi/gkernel.hpp> // GKernelPackage
#include <opencv2/gapi/infer.hpp>   // Generic
#include <opencv2/gapi/infer/ov.hpp>// workload_type

namespace cv {
namespace gapi {

/**
 * @brief This namespace contains G-API ONNX Runtime backend functions, structures, and symbols.
 */
namespace onnx {

/**
 * @brief This namespace contains Execution Providers structures for G-API ONNX Runtime backend.
 */
namespace ep {

/**
 * @brief This structure provides functions
 * that fill inference options for ONNX CoreML Execution Provider.
 * Please follow https://onnxruntime.ai/docs/execution-providers/CoreML-ExecutionProvider.html#coreml-execution-provider
 */
struct GAPI_EXPORTS_W_SIMPLE CoreML {
    /** @brief Class constructor.

    Constructs CoreML parameters.

    */

Review comment (suggestion):
I think that these two backend shouldn't reuse the same structure. How can we specify two different compile arguments for different backends that way... They might be inherited from the same structure, though.

Code after:
// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.
//
// Copyright (C) 2020-2021 Intel Corporation

#ifndef OPENCV_GAPI_INFER_ONNX_HPP
#define OPENCV_GAPI_INFER_ONNX_HPP

#include <unordered_map>
#include <string>
#include <array>
#include <tuple> // tuple, tuple_size
#include <map>

#include <opencv2/gapi/opencv_includes.hpp>
#include <opencv2/gapi/util/any.hpp>
#include <opencv2/gapi/util/optional.hpp>

#include <opencv2/core/cvdef.h>     // GAPI_EXPORTS
#include <opencv2/gapi/gkernel.hpp> // GKernelPackage
#include <opencv2/gapi/infer.hpp>   // Generic
#include <opencv2/gapi/infer/workload_type.hpp>

namespace cv {
namespace gapi {

/**
 * @brief This namespace contains G-API ONNX Runtime backend functions, structures, and symbols.
 */
namespace onnx {

/**
 * @brief This namespace contains Execution Providers structures for G-API ONNX Runtime backend.
 */
namespace ep {

/**
 * @brief This structure provides functions
 * that fill inference options for ONNX CoreML Execution Provider.
 * Please follow https://onnxruntime.ai/docs/execution-providers/CoreML-ExecutionProvider.html#coreml-execution-provider
 */
struct GAPI_EXPORTS_W_SIMPLE CoreML {
    /** @brief Class constructor.

    Constructs CoreML parameters.

    */
Repository: opencv/opencv
File: modules/gapi/include/opencv2/gapi/gcommon.hpp (C++)

Code before:
 * Specifies a number of threads that should be used by executor.
 */
struct GAPI_EXPORTS use_threaded_executor
{
    use_threaded_executor();
    explicit use_threaded_executor(const uint32_t nthreads);

    uint32_t num_threads;
};

namespace detail
{
    template<> struct CompileArgTag<cv::graph_dump_path>
    {
        static const char* tag() { return "gapi.graph_dump_path"; }
    };

    template<> struct CompileArgTag<cv::use_threaded_executor>
    {
        static const char* tag() { return "gapi.threaded_executor"; }
    };
}

class WorkloadType {
    using Callback = std::function<void(const std::string &type)>;
    std::unordered_map<unsigned int, Callback> listeners;
    unsigned int next_id = 0;
 public:
    unsigned int addListener(Callback cb) {
        unsigned int id = next_id++;
        listeners.emplace(id, std::move(cb));
        return id;
    }
    void removeListener(unsigned int id) {
        listeners.erase(id);
    }
    void set(const std::string &type) {
        for(const auto &listener : listeners) {
            listener.second(type);
        }
    }
};

} // namespace cv

// std::hash overload for GShape
namespace std
{
template<> struct hash<cv::GShape>
{
    size_t operator() (cv::GShape sh) const

Review comment (suggestion):
Better to place `private` variables at the end of the class with explicit usage of `private` access modifier just for consistency with other code

Code after:
 */
struct GAPI_EXPORTS use_threaded_executor
{
    use_threaded_executor();
    explicit use_threaded_executor(const uint32_t nthreads);

    uint32_t num_threads;
};

namespace detail
{
    template<> struct CompileArgTag<cv::graph_dump_path>
    {
        static const char* tag() { return "gapi.graph_dump_path"; }
    };

    template<> struct CompileArgTag<cv::use_threaded_executor>
    {
        static const char* tag() { return "gapi.threaded_executor"; }
    };
}

} // namespace cv

// std::hash overload for GShape
namespace std
{
template<> struct hash<cv::GShape>
{
    size_t operator() (cv::GShape sh) const
    {
        return std::hash<int>()(static_cast<int>(sh));
    }
};
} // namespace std


#endif // OPENCV_GAPI_GCOMMON_HPP
Repository: opencv/opencv
File: modules/gapi/include/opencv2/gapi/infer/onnx.hpp (C++)

Code before:
    void cfgSessionOptions(const std::map<std::string, std::string>& options) {
        desc.session_options.insert(options.begin(), options.end());
    }

/** @see onnx::Params::cfgOptLevel. */
    void cfgOptLevel(const int opt_level) {
        desc.opt_level = cv::util::make_optional(opt_level);
    }

    // BEGIN(G-API's network parametrization API)
    GBackend      backend() const { return cv::gapi::onnx::backend(); }
    std::string   tag()     const { return m_tag; }
    cv::util::any params()  const { return { desc }; }
    // END(G-API's network parametrization API)
protected:
    detail::ParamDesc desc;
    std::string m_tag;
};

class WorkloadTypeONNX : public WorkloadType {};
using WorkloadTypeOnnxPtr = std::shared_ptr<cv::gapi::onnx::WorkloadTypeONNX>;

} // namespace onnx
} // namespace gapi
namespace detail {
template<> struct CompileArgTag<std::shared_ptr<cv::gapi::onnx::WorkloadTypeONNX>> {
    static const char* tag() { return "gapi.wip.ov.workload_type_onnx_ptr"; }
};
} // namespace detail
} // namespace cv

#endif // OPENCV_GAPI_INFER_HPP


Review comment (suggestion):
I think you can safely use `WorkloadTypeOnnxPtr` here

Code after:
    /** @see onnx::Params::cfgSessionOptions. */
    void cfgSessionOptions(const std::map<std::string, std::string>& options) {
        desc.session_options.insert(options.begin(), options.end());
    }

/** @see onnx::Params::cfgOptLevel. */
    void cfgOptLevel(const int opt_level) {
        desc.opt_level = cv::util::make_optional(opt_level);
    }

    // BEGIN(G-API's network parametrization API)
    GBackend      backend() const { return cv::gapi::onnx::backend(); }
    std::string   tag()     const { return m_tag; }
    cv::util::any params()  const { return { desc }; }
    // END(G-API's network parametrization API)
protected:
    detail::ParamDesc desc;
    std::string m_tag;
};

class WorkloadTypeONNX : public WorkloadType {};
using WorkloadTypeONNXPtr = std::shared_ptr<cv::gapi::onnx::WorkloadTypeONNX>;

} // namespace onnx
} // namespace gapi
namespace detail {
template<> struct CompileArgTag<cv::gapi::onnx::WorkloadTypeONNXPtr> {
    static const char* tag() { return "gapi.onnx.workload_type"; }
};
} // namespace detail
} // namespace cv

#endif // OPENCV_GAPI_INFER_HPP
Repository: opencv/opencv
File: modules/gapi/include/opencv2/gapi/infer/onnx.hpp (C++)

Code before:
    void cfgDisableMemPattern() {
        desc.disable_mem_pattern = true;
    }

    /** @see onnx::Params::cfgSessionOptions. */
    void cfgSessionOptions(const std::map<std::string, std::string>& options) {
        desc.session_options.insert(options.begin(), options.end());
    }

/** @see onnx::Params::cfgOptLevel. */
    void cfgOptLevel(const int opt_level) {
        desc.opt_level = cv::util::make_optional(opt_level);
    }

    // BEGIN(G-API's network parametrization API)
    GBackend      backend() const { return cv::gapi::onnx::backend(); }
    std::string   tag()     const { return m_tag; }
    cv::util::any params()  const { return { desc }; }
    // END(G-API's network parametrization API)
protected:
    detail::ParamDesc desc;
    std::string m_tag;
};

class WorkloadTypeONNX : public WorkloadType {};
using WorkloadTypeOnnxPtr = std::shared_ptr<cv::gapi::onnx::WorkloadTypeONNX>;

} // namespace onnx
} // namespace gapi
namespace detail {
template<> struct CompileArgTag<std::shared_ptr<cv::gapi::onnx::WorkloadTypeONNX>> {
    static const char* tag() { return "gapi.wip.ov.workload_type_onnx_ptr"; }
};
} // namespace detail
} // namespace cv

#endif // OPENCV_GAPI_INFER_HPP


Review comment (suggestion):
If we preserve this `using`, suggest to have `ONNX` with capitals here to be aligned with the type name above

Code after:
    /** @see onnx::Params::cfgDisableMemPattern. */
    void cfgDisableMemPattern() {
        desc.disable_mem_pattern = true;
    }

    /** @see onnx::Params::cfgSessionOptions. */
    void cfgSessionOptions(const std::map<std::string, std::string>& options) {
        desc.session_options.insert(options.begin(), options.end());
    }

/** @see onnx::Params::cfgOptLevel. */
    void cfgOptLevel(const int opt_level) {
        desc.opt_level = cv::util::make_optional(opt_level);
    }

    // BEGIN(G-API's network parametrization API)
    GBackend      backend() const { return cv::gapi::onnx::backend(); }
    std::string   tag()     const { return m_tag; }
    cv::util::any params()  const { return { desc }; }
    // END(G-API's network parametrization API)
protected:
    detail::ParamDesc desc;
    std::string m_tag;
};

class WorkloadTypeONNX : public WorkloadType {};
using WorkloadTypeONNXPtr = std::shared_ptr<cv::gapi::onnx::WorkloadTypeONNX>;

} // namespace onnx
} // namespace gapi
namespace detail {
template<> struct CompileArgTag<cv::gapi::onnx::WorkloadTypeONNXPtr> {
    static const char* tag() { return "gapi.onnx.workload_type"; }
};
} // namespace detail
} // namespace cv

#endif // OPENCV_GAPI_INFER_HPP
Repository: opencv/opencv
File: modules/dnn/include/opencv2/dnn/all_layers.hpp (C++)

Code before:
    };

    class CV_EXPORTS DequantizeLinearLayer : public Layer
    {
    public:
        int axis;
        int block_size;

        static Ptr<DequantizeLinearLayer> create(const LayerParams& params);
    };

    class CV_EXPORTS RequantizeLayer : public Layer
    {
    public:
        float scale, shift;
        static Ptr<RequantizeLayer> create(const LayerParams &params);
    };

    class CV_EXPORTS IfLayer : public Layer
    {
    public:
        /** Factory: creates an IfLayer implementation. */
        static Ptr<IfLayer> create(const LayerParams& params);

        /** Returns the selected subgraph based on the boolean flag. */
        virtual Ptr<Graph> then_else(bool flag) const = 0;
    };

    class CV_EXPORTS ConcatLayer : public Layer
    {
    public:
        int axis;
        /**
         * @brief Add zero padding in case of concatenation of blobs with different
         * spatial sizes.
         *
         * Details: https://github.com/torch/nn/blob/master/doc/containers.md#depthconcat
         */
        bool padding;
        int paddingValue;

        static Ptr<ConcatLayer> create(const LayerParams &params);
    };

    class CV_EXPORTS Concat2Layer : public Layer
    {
    public:
        int axis;

        static Ptr<Concat2Layer> create(const LayerParams &params);
    };

Review comment (suggestion):
IfLayer should override Layer::subgraphs() method. `then_else()` method is not needed, please, remove it. 

Code after:
    };

    class CV_EXPORTS DequantizeLinearLayer : public Layer
    {
    public:
        int axis;
        int block_size;

        static Ptr<DequantizeLinearLayer> create(const LayerParams& params);
    };

    class CV_EXPORTS RequantizeLayer : public Layer
    {
    public:
        float scale, shift;
        static Ptr<RequantizeLayer> create(const LayerParams &params);
    };

    // Forward declaration for computational Graph used by IfLayer
    class Graph;

    class CV_EXPORTS IfLayer : public Layer
    {
    public:
        virtual int branch(InputArray arr) const = 0;

        /** Factory: creates an IfLayer implementation. */
        static Ptr<IfLayer> create(const LayerParams& params);
    };

    class CV_EXPORTS ConcatLayer : public Layer
    {
    public:
        int axis;
        /**
         * @brief Add zero padding in case of concatenation of blobs with different
         * spatial sizes.
         *
         * Details: https://github.com/torch/nn/blob/master/doc/containers.md#depthconcat
         */
        bool padding;
        int paddingValue;

        static Ptr<ConcatLayer> create(const LayerParams &params);
    };

    class CV_EXPORTS Concat2Layer : public Layer
    {
    public:
        int axis;
Repository: opencv/opencv
File: modules/dnn/src/layers/topk2_layer.cpp (C++)

Code before:
// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.

#include "../precomp.hpp"
#include <opencv2/dnn/shape_utils.hpp>

namespace cv { namespace dnn {

namespace {

template<typename T>
class ComparatorGreater {
public:
    ComparatorGreater(const T* data, size_t step)
        : data_(data), step_(step) {}

    void addOffset(size_t offset) {
        data_ += offset;
    }

    void minusOffset(size_t offset) {
        data_ -= offset;
    }

    bool operator()(const size_t lhs_idx, const size_t rhs_idx) {
        T lhs = *(data_ + lhs_idx * step_),
          rhs = *(data_ + rhs_idx * step_);
        return (lhs > rhs || (lhs == rhs && lhs_idx < rhs_idx));
    }

private:
    const T* data_;
    size_t step_;
};

template<typename T>
class ComparatorLess {
public:
    ComparatorLess(const T* data, size_t step)
        : data_(data), step_(step) {}

    void addOffset(size_t offset) {
        data_ += offset;
    }

    void minusOffset(size_t offset) {

Review comment (suggestion):
I suggest to avoid using addOffset() and minusOffset(). Construct ComparatorGreater/ComparatorLess inside the inner loop, this is super-cheap operation, and pass the proper data there.

Code after:
// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.

#include "../precomp.hpp"
#include <opencv2/dnn/shape_utils.hpp>

namespace cv { namespace dnn {

/*
    TopK layer, as defined in ONNX specification:
    https://onnx.ai/onnx/operators/onnx__TopK.html

    Opset’s 1, 10 and 11 are covered.
*/

namespace {

template<typename T, typename WT = T>
class ComparatorGreater {
public:
    using value_type = std::pair<T, WT>;
    bool operator()(const value_type& a, const value_type& b) const {
        return (a.second > b.second || (a.second == b.second && a.first < b.first));
    }
};

template<typename T, typename WT = T>
class ComparatorLess {
public:
    using value_type = std::pair<T, WT>;
    bool operator()(const value_type& a, const value_type& b) const {
        return (a.second < b.second || (a.second == b.second && a.first < b.first));
    }
};
}

class TopK2LayerImpl CV_FINAL : public TopK2Layer
{
public:
    int axis;
    bool largest;
    bool sorted;
    int K;
    bool dynamicK;

    TopK2LayerImpl(const LayerParams& params)
Repository: opencv/opencv
File: modules/dnn/src/layers/topk2_layer.cpp (C++)

Code before:

template<typename T, typename WT = T>
class ComparatorGreater {
public:
    ComparatorGreater(const T* data, size_t step)
        : data_(data), step_(step) {}


    bool operator()(const size_t lhs_idx, const size_t rhs_idx) {
        WT lhs = static_cast<WT>(*(data_ + lhs_idx * step_));
        WT rhs = static_cast<WT>(*(data_ + rhs_idx * step_));
        return (lhs > rhs || (lhs == rhs && lhs_idx < rhs_idx));
    }

private:
    const T* data_;
    size_t step_;
};

template<typename T, typename WT = T>
class ComparatorLess {
public:
    ComparatorLess(const T* data, size_t step)
        : data_(data), step_(step) {}

    bool operator()(const size_t lhs_idx, const size_t rhs_idx) {
        WT lhs = static_cast<WT>(*(data_ + lhs_idx * step_));
        WT rhs = static_cast<WT>(*(data_ + rhs_idx * step_));
        return (lhs < rhs || (lhs == rhs && lhs_idx < rhs_idx));
    }

private:
    const T* data_;
    size_t step_;
};
}

class TopK2LayerImpl CV_FINAL : public TopK2Layer
{
public:
    int axis;
    bool largest;
    bool sorted;
    int K;
    bool dynamicK;

    TopK2LayerImpl(const LayerParams& params)
    {
        setParamsFrom(params);
        axis    = params.get<int>("axis", -1);
        largest = params.get<int>("largest", 1) == 1;

Review comment (suggestion):
modify it to take 2 `std::pair<>` and don't use indirect access to data:

```
template <typename T, typename VT>
class ComparatorLess {
     using value_type = std::pair<T, VT>
     bool operator() (const value_type& a, const value_type& b) const {
          return (a.second < b.second || (a.second == b.second && a.first < b.first);
    }
};
...
```


Code after:

template<typename T, typename WT = T>
class ComparatorGreater {
public:
    using value_type = std::pair<T, WT>;
    bool operator()(const value_type& a, const value_type& b) const {
        return (a.second > b.second || (a.second == b.second && a.first < b.first));
    }
};

template<typename T, typename WT = T>
class ComparatorLess {
public:
    using value_type = std::pair<T, WT>;
    bool operator()(const value_type& a, const value_type& b) const {
        return (a.second < b.second || (a.second == b.second && a.first < b.first));
    }
};
}

class TopK2LayerImpl CV_FINAL : public TopK2Layer
{
public:
    int axis;
    bool largest;
    bool sorted;
    int K;
    bool dynamicK;

    TopK2LayerImpl(const LayerParams& params)
    {
        setParamsFrom(params);
        axis    = params.get<int>("axis", -1);
        largest = params.get<int>("largest", 1) == 1;
        sorted  = params.get<int>("sorted", 1) == 1;
        CV_CheckTrue(sorted, "TopK2: sorted == false is not supported");
        if (params.has("k")) {
            K = params.get<int>("k");
            CV_CheckGT(K, 0, "TopK2: K needs to be a positive integer");
            dynamicK = false;
        } else {
            dynamicK = true;
            K = 0;
        }
    }

    bool supportBackend(int backendId) CV_OVERRIDE
    {
        return backendId == DNN_BACKEND_OPENCV;
    }