Skip to content

Commit

Permalink
getScreenRGB allocation bugfix
Browse files Browse the repository at this point in the history
In atari_py/ale_c_wrapper.h:62 it writes 4 bytes per pixel, but was
allocating 3 bytes here.
Gym wasn’t affected, because it pre-allocates a buffer
  • Loading branch information
tlbtlbtlb committed Mar 14, 2017
1 parent d63ced3 commit a802482
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions atari_py/ale_python_interface.py
Expand Up @@ -193,13 +193,13 @@ def getScreen(self, screen_data=None):
def getScreenRGB(self, screen_data=None):
"""This function fills screen_data with the data in RGB format
screen_data MUST be a numpy array of uint8. This can be initialized like so:
screen_data = np.empty((height,width,3), dtype=np.uint8)
screen_data = np.empty((height,width,4), dtype=np.uint8)
If it is None, then this function will initialize it.
"""
if(screen_data is None):
width = ale_lib.getScreenWidth(self.obj)
height = ale_lib.getScreenHeight(self.obj)
screen_data = np.empty((height, width,3), dtype=np.uint8)
screen_data = np.empty((height, width,4), dtype=np.uint8)
ale_lib.getScreenRGB(self.obj, as_ctypes(screen_data[:]))
return screen_data

Expand Down

10 comments on commit a802482

@Kojoley
Copy link

Choose a reason for hiding this comment

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

Several people (and me too) struck into this previously. This could be simply fixed, but requires simultaneous fix to gym. I can prepare a PR if you are interested.

@tlbtlbtlb
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'm not sure what you have in mind. Is there still a bug?

@Kojoley
Copy link

Choose a reason for hiding this comment

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

Well, function name is getScreenRGB, but it returns BGRX (reverted order + extra byte), so technically speaking yeah, there is still a bug.

@tlbtlbtlb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming the function will break things, so I'll just document it clearly.

@Kojoley
Copy link

Choose a reason for hiding this comment

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

So you think it is better to leave this behavior and a workaround gym? I do not understand...

@tlbtlbtlb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@Kojoley
Copy link

Choose a reason for hiding this comment

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

On my system those byte swaps consume about the half of forward pass time. So it is nice way to spend cpu time.

@tlbtlbtlb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where are they being swapped? They don't get swapped in Gym or for most Gym agents I know about. OpenCV actually requires BGR order, and most Gym agents start by resizing the image.

@Kojoley
Copy link

Choose a reason for hiding this comment

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

@tlbtlbtlb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right: there was an extra swap going on (although not where you point out: numpy can do that operation without touching the data by changing strides.) Anyway, these 2 commits (now in master) give a significant performance improvement:

openai/gym@3bc4692
5f2a362

Please sign in to comment.