Skip to content

[Bounding box] Print label in center of generated image.#9

Open
aelkhour wants to merge 1 commit intoarterys:masterfrom
aelkhour:bounding_box_label
Open

[Bounding box] Print label in center of generated image.#9
aelkhour wants to merge 1 commit intoarterys:masterfrom
aelkhour:bounding_box_label

Conversation

@aelkhour
Copy link
Contributor

@aelkhour aelkhour commented Apr 5, 2020

This is quite handy for binary classifiers.

0_COVID-19_7w7ujnj3 dcm

2__0_6256933_ujx38wix dcm

@bsblaw
Copy link
Contributor

bsblaw commented Apr 14, 2020

@aelkhour When I tried it out, I get the following error:

benl@benl-02:~/ws/aelkhour-inference-sdk/inference-test-tool$ ./send-inference-request.sh -b --host 0.0.0.0 --port 8900 ./study1/
Sending 1 files...
JSON response: {'parts': [], 'bounding_boxes_2d': [{'SOPInstanceUID': '1.2.392.200046.100.13.24550048020077643303031580933179026570', 'top_left': [5, 5], 'label': 'super bbox', 'bottom_right': [10, 10]}], 'protocol_version': '1.0'}
Traceback (most recent call last):
File "run.py", line 143, in
upload_study_me(args.file_path, model_type, args.host, args.port)
File "run.py", line 124, in upload_study_me
test_inference_boxes.generate_images_with_boxes(images, boxes, output_folder)
File "/opt/test_inference_boxes.py", line 34, in generate_images_with_boxes
font = ImageFont.truetype('OpenSans-Regular.ttf', br[1] // 20)
File "/usr/local/lib/python3.8/site-packages/PIL/ImageFont.py", line 642, in truetype
return freetype(font)
File "/usr/local/lib/python3.8/site-packages/PIL/ImageFont.py", line 639, in freetype
return FreeTypeFont(font, size, index, encoding, layout_engine)
File "/usr/local/lib/python3.8/site-packages/PIL/ImageFont.py", line 187, in init
self.font = core.getfont(
OSError: cannot open resource

@bsblaw
Copy link
Contributor

bsblaw commented Apr 14, 2020

I think you just need to include a font file in the inference-test-tool/fonts directory or something and then you should be able to just do font = ImageFont.truetype('/opt/fonts/FreeSans.ttf', br[1] // 20).

@aelkhour
Copy link
Contributor Author

I think you just need to include a font file in the inference-test-tool/fonts directory or something and then you should be able to just do font = ImageFont.truetype('/opt/fonts/FreeSans.ttf', br[1] // 20).

Sorry for the late reply @bsblaw , I somehow missed your comments.
Indeed I forget to commit that part. I added an install step for the font in the Dockerfile, let me know if it's ok for you.

@aelkhour
Copy link
Contributor Author

@bsblaw If you have time and think this PR could be useful, could you retest the updated version please?


draw.line(points, fill='red', width=5)
label = box['label']
font = ImageFont.truetype('OpenSans-Regular.ttf', br[1] // 20)
Copy link
Contributor

@mats-claassen mats-claassen May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case br[1] // 20 is the font size right? Now, br[1] is the y coordinate of the bottom right point. That means that boxes that are further down on the image will have a bigger text than boxes at the top. This might work fine for classification but let's make it work for bounding boxes as well.

Maybe dcm.Rows // 20?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is not very robust.

I made a slightly different proposal relying on the bounding box size instead of the image size, the rationale being that if in the future we want to support multiple bounding boxes in the same image, the labels should be positioned accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually support multiple bounding boxes in the same image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see with using the text size relative to bbox size is that for small boxes with, say 20px height, the text would be 1px height and unreadable. That is why a fixed font size would be better.
Other implementations I have seen show the text above the bbox, instead of inside. In that case there would be no issue if the text is bigger than the box

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you that this PR handles poorly all edge cases, as my intent was to focus on the most common use case (IMHO) of creating a single bbox which has the same size as the image. In that specific case, placing the label above the bbox sadly leads to a text lying outside of the image.

The worst case scenario for this PR would be having a text label being too small to read, somewhat equivalent to running the inference without this PR. What do you think of keeping it this way as I don't see any negative effect?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point regarding placing the label on top of the bounding box. You are right there. However, we are using bboxes for classification as a workaround currently. Ideally we should agree on a format for classification which does not need a bbox. @bsblaw What do you think?

label = box['label']
font = ImageFont.truetype('OpenSans-Regular.ttf', br[1] // 20)
text_width, text_height = draw.textsize(label, font)
text_position = ((br[0] - text_width) / 2, (br[1] - text_height) / 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same happens to this calculation. It does not work for bounding boxes that do not span the whole image.

@aelkhour aelkhour force-pushed the bounding_box_label branch from 1d06a86 to b67d535 Compare May 17, 2020 15:00
@aelkhour aelkhour force-pushed the bounding_box_label branch from b67d535 to f214752 Compare May 17, 2020 15:16
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.

3 participants