OpenCV API is primary C++ based. However we need to think about Python, Java bindings - and support them too.

@alalek I am planning to do Python and Java bindings after we finalize this API interface. Thanks for the reminder!

@ocpalo, thank you! Please, rename PR so that the title contains [GSoC 2022] in the beginning.

I think I can remove the cv::Exception part from the API and provide error codes. This way, API would be more efficient.

@ocpalo, overall, it looks good now. I put a couple of small comments.

ok, I realized that init() reassigned m_decoder, so it should be closed automatically.

I made the fixes. Please take a look again @vpisarev @asmorkalov.

ALso, the Windows10-x64 fail is not related with my pull request. Please rerun that workflow only if you can. I need to push commit to re-run all workflows.

at() calls operator[], but not vise versa. In case if index is -1 you'll get out of bound access for std::vector. For index, let say 1e+6 the next loop will be iterated 1e+6 times: for(int i = 0; i != index && advance(); ++i) {}. I propose to move the check from at() to operator[] or have it in both methods. In std::vector<> is reasonable to get rid of range check for performance reason. Range check is heavier than actual work. But it's not true for image codecs. Also it could be a security issue.

@asmorkalov Regarding this comment, I have changed the std::vector operator[] with the std::vector .at() method. In this implementation, I pre-allocate vectors size. Now, In the Impl class, it does out of range access check in the both cases. I think that is good enough for range checks.

Somehow i can not reply to your comment. That is why i wrote here as seperate comment. Thanks for the dealing with the CI issue!

Mat& ImageCollection::Impl::operator[](int index) {
    if(m_pages.at(index).empty()) {

The current iterator api does not implements operator->. Should I?

struct CV_EXPORTS iterator {
     Mat* operator->();
     ...
}

Mat* ImageCollection::iterator::operator->() {
    CV_Assert(m_pCollection);
    return &m_pCollection->getImpl()->operator[](m_curr);
}

So one can use this to do operations on the cache Mat objects. And I realized that my at() and operator[] methods returns copy of the Mat object. I think I should return Mat& to get rid of expensive copy costs in case of return value is reference to Mat.

@asmorkalov , Could you take a look at my last commit and tell me what do you think?

I have checked the std::vector::at() implementation. This also returns reference type.

I also pushed the test that Vadim sent me to see if operator++ is works as expected. Since both of you suspected that it is a bug.

Edit: CI fails does not related with my commits again.

Further explanation about operator++() advance call. There are 2 variables named m_curr and m_current. Maybe I should rename them.

• m_curr -> Iterator API
• m_current -> ImageCollection API

When operator++ is called, either way, we need to increment m_curr to understand that iterator points to which page.
On the other hande, m_current can be modified only if m_current is equal to m_curr. Suppose we created a new ImageCollection and Iterator. So both m_curr and m_current is initialized to 0. If we advance with operator++(), both will increase its internal variables because they are equal. Now both variable are equal to 1. Lets say we create another iterator, iterB, . This iterator m_current is initialized to 0. If we call operator++ on iterator iterB, it will not advance the page (m_current) because it is not equal to iterB's m_curr. It should not because iterB is behind of the first iterator.

So what happens if iterB is tries to decode page 1(index 0) without calling operator++(). In this case, ImageCollection's m_decoder will be reinitialized to points to first page. Then it will advance the pages until iterB's m_current is equal to m_curr. Then it decodes the page. In this way, I keep the m_current determined. Even thought, one iterator goes beyond of the scope, if other iterator tries to decode, the ImageCollection will be reinitialized and will advance until that index.

Further explanation about operator++() advance call. There are 2 variables named m_curr and m_current. Maybe I should rename them.

m_curr -> Iterator API
m_current -> ImageCollection API

Rename - it's good idea.

it makes sense to return const cv::Mat& in at and operator[] to not poison channels, size, etc on user side.

@ocpalo Please make operator[] and at() return values constant reference and squash commits. I'll merge the patch then.

Ok. Give me an hour or two.

Thank you for the reviews btw.

@asmorkalov I have made the changes and pushed. Tbh, I do not understand why we force users to use const& if they want to reference it.

If we keep operator[] and at() as before, the user can use it like this

cv::Mat& firstPage = collection[0]; // 1
const cv::Mat& cFirstPage = collection[0]; //2

After the changes, the first one(1) would not compile. I understand the reason that it is protection for the user but we should not do that because since he is taking it by reference, it is his reponsibility to deal with this kind of stuff.

It's weak protection, but it covers at least:

• type, shape, channels change.
• unexpected cv::Mat realloc, if user tries to put that mat as output argument.
  Mat content is still in danger, but out-of-bound access will not happen, if some piece of code relies on saved width and height.

@ocpalo, @asmorkalov, thank you for polishing it! I think, it can be merged now