fix "ValueError: too many values to unpack" at cv2.findContours

[katotetsuro]

This PR fix the issue vis_one_image_opencv raises ValueError.

Since OpenCV 3.2, findContours() no longer modifies the source image but returns a modified image as the first of three return parameters.
https://docs.opencv.org/3.3.1/d4/d73/tutorial_py_contours_begin.html

and this line also assumes opencv>=3.2
https://github.com/facebookresearch/Detectron/blob/master/lib/utils/vis.py#L326

so, It may be better to set OpenCV version specification to 3.2 or higher in Installation guide.

[jasonbunk]

It could also be made backwards compatible with OpenCV 2, with a try/except like this:
jasonbunk@0f4589d
Or, perhaps instead it could check cv2.__version__ and act accordingly. I tested the above commit by running tools/infer_simple.py with the demo on OpenCV 2.4.13 and it worked.

[ir413]

Thanks for the PR @katotetsuro, looks great!

Thanks for the suggestion @jasonbunk. Providing backward compatibility with OpenCV 2 is outside the scope of this PR. I'm also not sure if there is a net benefit in maintaining the backward compatibility but we can discuss that in another issue/PR.

[ir413]

@caffe2bot test this please.

[facebook-github-bot]

@ir413 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ir413]

Hi @katotetsuro, we're having some issues with merging this PR internally. Could you please rebase the changes onto a fresh pull of Detectron? Sorry for the inconvenience.

[katotetsuro]

Hi @ir413,
I tried git pull --rebase origin master in my fix-vis_mask branch, and I pushed to my fork repo.
Could you review again? Thanks!

[facebook-github-bot]

@ir413 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ir413]

Thanks @katotetsuro, the update looks great!

[rbgirshick]

LGTM

[aishwaryap]

With opencv 4.0.0 the convention seems to have changed again so this fix breaks. Downgrading to 3.2.0 fixed it for me. It would be good to reflect this in the requirements.

[ppwwyyxx]

Thanks @aishwaryap . I've made #847 which should have addressed this.