Skip to content

Comments

Feed the ImageDataLayer with OpenCV images directly from memory#251

Closed
kloudkl wants to merge 11 commits intoBVLC:devfrom
kloudkl:images_from_memory
Closed

Feed the ImageDataLayer with OpenCV images directly from memory#251
kloudkl wants to merge 11 commits intoBVLC:devfrom
kloudkl:images_from_memory

Conversation

@kloudkl
Copy link
Contributor

@kloudkl kloudkl commented Mar 23, 2014

The discussion of #196 led to the conclusion that the ImagesLayer (#120) by @sguada is more suitable for ingesting raw images. Following @Yangqing's suggestion, this PR extends ImagesLayer to accept OpenCV format images which are used by computer vision researchers and engineers very commonly.

@kloudkl
Copy link
Contributor Author

kloudkl commented Mar 25, 2014

Rebased and ready for being merged.

@kloudkl
Copy link
Contributor Author

kloudkl commented Apr 3, 2014

@shelhamer, this has been rebased and tested again. Please invite a reviewer. Thanks!

@shelhamer
Copy link
Member

@sguada could you review this since you reviewed #196 and have been working on the data layers?

@kloudkl could you squash 713bb18, 148dbdb, and 6354a65 to clean up the history? Just a minor style point.

@kloudkl
Copy link
Contributor Author

kloudkl commented Apr 4, 2014

Squashed and tested.

@shelhamer
Copy link
Member

@longjon is this superseded by #294? Thanks for the work even if so @kloudkl.

@longjon
Copy link
Contributor

longjon commented May 2, 2014

@shelhamer, this PR understands two things which #294 does not:

  • OpenCV datatypes
  • std::vectors and discontinuous data

If this functionality is desired in caffe, it probably makes the most sense to generalize #294 in some way.

@onauparc
Copy link

@kloudkl could you resolve conflicts? I'd like to give it a try.

@kloudkl
Copy link
Contributor Author

kloudkl commented Jun 24, 2014

@onauparc, you can try it now.

@shelhamer shelhamer assigned longjon and unassigned sguada Jun 24, 2014
@shelhamer
Copy link
Member

@longjon I've heard a desire for OpenCV input come up a few times now. Could you review this and decide whether to merge or guide adapting this into a generalization of #294 ? Thanks!

@kloudkl kloudkl changed the title Feed ImagesLayer with OpenCV images directly from memory Feed the ImageDataLayer with OpenCV images directly from memory Jun 24, 2014
@kloudkl
Copy link
Contributor Author

kloudkl commented Jun 24, 2014

The ImageDataLayer does a series of necessary pre-processing steps. The more general solution would probably have to wait until @Yangqing's new design is finalized.

@longjon
Copy link
Contributor

longjon commented Jun 25, 2014

@shelhamer (and others): If I read correctly, this patch modifies ImageDataLayer by implementing OpenCVImageToDatum, which duplicates ReadImageToDatum, omitting the file reading step.

This all seems a bit convoluted; we start with a cv::Mat, which is row-major storage, then convert to a protobuf format, then back to a row-major blob! This is true both for this patch and for the existing ImageDataLayer. (I guess there is an implicit transposition, but that should be abstracted anyway).

I think this should be folded into @Yangqing's plan at #407 (comment). Of course, neither this (nor MemoryDataLayer) probably need prefetching, but it doesn't hurt. This layer would be implemented by simply stepping over a vector<cv::Mat>, similar to the way MemoryDataLayer is implemented. ImageDataLayer would be implemented similarly, except the cv::Mats would be read from disk. ReadImageToDatum would go away.

One can read from cv::Mats today, using MemoryDataLayer, either updating the data blob-at-a-time or copying all data into contiguous memory. Yes, one has to do preprocessing that DataLayer currently does oneself, but since we seem to moving in the direction of preprocessing outside the data layer, (which I totally support), it might be worthwhile to just implement desired in-network preprocessing using existing (or new) layers.

@shelhamer
Copy link
Member

@longjon totally agree. @Yangqing's data layer refactoring will be the foundation to bring this all together, separate pre-processing, and make our lives easier.

@bhack
Copy link
Contributor

bhack commented Jul 23, 2014

@longjon I agree that is not efficient. So.. do we need to wait for #407?

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 9, 2014

In the latest ImageDataLayer, the OpenCV images in the Mat format are transformed into Blobs directly and the source parameter doesn't have to be set anymore (#499 (comment)).

Until now, the data layer refactoring project has made no observable progress. This PR is still the only available implementation to automatically transform the OpenCV images and pass them into the network. Would you please review this again and decide whether to merge or not?

@shelhamer
Copy link
Member

@longjon please review and decide on merge. Thanks for the rebase and alternative data layer work @kloudkl.

@bhack
Copy link
Contributor

bhack commented Aug 22, 2014

@kloudkl I think that #954 merged in dev impact this PR.

@bhack
Copy link
Contributor

bhack commented Aug 22, 2014

I think that this is one of the cases when a fresher PR charges some re-basing work to an older PR only because it was reviewed faster. I know that like other open source projects review are done best effort but I think that we could implement some little policy to incentive PR maintaining especially when we could have PR opened for months with cross impact.

sh1r0 added a commit to sh1r0/caffe that referenced this pull request Aug 24, 2014
@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 27, 2014

@bhack, now that #954 extracted the duplicated data transformations out of the old data layers, the purpose of this PR can be better achieved with the new DataTransformer and the MemoryDataLayer as @shelhamer suggested in #941. So this should be closed.

@kloudkl kloudkl closed this Aug 27, 2014
@bhack
Copy link
Contributor

bhack commented Aug 27, 2014

@kloudkl If you want to remove this can you add a little doc on how to interface Opencv mat with MemoryDataLayer?

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 29, 2014

Development continues in #995.

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.

6 participants