Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New cluster #43

Open
wants to merge 20 commits into
base: phase2
Choose a base branch
from
Open

New cluster #43

wants to merge 20 commits into from

Conversation

JordanCheney
Copy link
Contributor

@nxwhite-str @carlosdcastillo @stephenrawls

This is a large commit but I hope it makes things easier for you as you implement clustering and addresses a few pain points. I will outline the major changes below:

  1. It replaces janus_media with an opaque janus_media_type pointer. An implementation has been provided in opencv_io that loads frames and images lazily so that a video no longer has to fit totally in RAM. Additionally, if you would like to have your own media object that is more efficient for your implementation you now have that freedom.
  2. The janus_association struct has been replaced by adding a janus_media pointer to janus_track. I think this is more clear for its purpose then the old janus_association object.
  3. The janus_template_id typedef was removed in favor of uint32_t (note it was previously a size_t).
  4. New cluster functions geared specifically towards tests 7 & 8 in the evaluation protocol.
  5. The evaluation harness is now built as it's own library instead of included in your deliveries. This makes it easier for us to change evaluation function implementations as required without name conflicts.
  6. Reorganization of the project. Harness functions have been moved to a new "harness" directory, example implementations (PP5 and opencv_io) to the "implementations" directory. The data directory was deleted because it was no longer in use.
  7. A number of changes in the harness to improve data parsing, provide more helpful command line inputs and make things generally more efficient.

Please let me know your thoughts, questions, concerns as you work through the review.

Best,
Jordan

JordanCheney and others added 17 commits November 30, 2016 16:29
…ry instead of as part of the implementation. Refactored verify to work in a lazy-loading fashion using the matches file to determine what templates to match
…tell if multiple frames come from the same source video.
…o metadata in 1N probe video and allow additional tracking information to be captured during janus_create_template
janus_template_id replaced with uint32_t
Updated cluster harness to support template lists and media lists
Added command line argument to janus_cluster utility to support template lists and media lists
Updated PP5 routines to handle lazy loading
Updated PP5 Clustering for template list support
Removed janus_association structure and put janus_media pointer in janus_track
Changed janus_create_template function prototype to qsupport std::vector<janus_track> instead of std::vector<janus_association>
Changed protocol parser to support one piece of metadata per subject per media
…w_cluster

Conflicts:
	include/iarpa_janus.h
	src/janus_io.cpp
	src/opencv_io/opencv_io.cpp
	src/opencv_io/opencv_io.h
@nxwhite-str
Copy link

@JordanCheney I have no problems with items 2-7, with the minor exception of #3 - changing the underlying type is fine, but I'd prefer to retain the typedef to keep our code clearer.

For #1, I think the change to providing an iterator and indexed access to video frames is great. On the other hand, if I'm reading the proposed implementation correctly, the only way we would have access to the janus_media interface is to either implement our own, or include one of the provided test harness implementations into our library. Instead of an opaque pointer to struct, it seems better to provide an abstract base class as an interface, and to have the janus_media implementations inherit from that.

Implementing our own janus_media would not be desirable for us - it would mean committing to supporting a fixed set of video container types and codecs, as well as image formats. For example, OpenCV doesn't support GIFs, so we would need to include an additional library to support that, and our code would not be future proof to new formats. It might also introduce licensing or patent issues with libraries we would need to distribute with our code. We would rather deal with raw pixels.

The second option for us would be including, e.g., opencv_io.h from the test harness to get the interface for that implementation. That would mean that a user of our library that wanted to change to a different janus_media implementation would need to edit and recompile our code, which also seems undesirable. I'd prefer defining a janus_media interface in iarpa_janus.h.

@JordanCheney
Copy link
Contributor Author

@nxwhite-str Thanks for the quick review, these are all valid points. For #1 we will bring back the typedef.

I didn't consider some of the license difficulties with opaque media pointers. I think your proposed public interface solves that problem cleanly. I'm envisioning two structures- janus_image which is basically a pointer to some data with size information and janus_media_iterator which implements the interface laid out in opencv_io.h. I will put the header together and add it here for review.

@stephenrawls
Copy link
Contributor

Hi @JordanCheney -- for clarification, is the plan to push this change prior to the upcoming API delivery at the end of January, or is this change going to happen afterwards?

Thanks,
Stephen Rawls (ISI)

@JordanCheney
Copy link
Contributor Author

@stephenrawls Hi Stephen, I'm sure you saw on the mailing list but this change will not effect the February delivery but will be for the final phase 2 delivery.

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.

4 participants