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

camera is warped #4

Closed
ofZach opened this issue Aug 16, 2017 · 42 comments
Closed

camera is warped #4

ofZach opened this issue Aug 16, 2017 · 42 comments

Comments

@ofZach
Copy link
Collaborator

ofZach commented Aug 16, 2017

as discussed here (and looks like it has started to be changed)

https://forum.openframeworks.cc/t/ycbcr-to-rgb-conversion-ios-and-arkit-related-via-glsl/27469/12

@sortofsleepy
Copy link
Owner

(including @stc)

so I started to work on this in camera-wraping(I know I know I'm sleepy) and I think I got it mostly working, there's only one very peculiar issue where the FBO won't render until an anchor gets added. I'm kinda at a loss with this at the moment.

Also I had encountered a very odd thing - I'm testing on an iPhone, which is unfortunately the only thing available to me at the moment. For me, as far as I can tell, I'm not getting a cropped image, even from a new barebones AR project with Metal.

(image with debug stuff is oF, image without is metal - upside down to try and ensure similar image using table for stability)
img_0008
img_0009

I have no idea why you guys were seeing a distorted image - could this just be an iPad only occurrence?

My device

  • iPhone 6s
  • Xcode beta 5
  • iOS 11 (15A5341f)

I do see my test device has an update to beta 5, that might be the difference, I can try again tomorrow.

@stc
Copy link
Collaborator

stc commented Aug 17, 2017

seems that the distorted image is depending on the aspect ratio:
the phone above has 1136/640 (=1.775) and the camera has 1280/720 (=1.777) which fits together well.
however the ipad pro has 2732/2048 (=1.333) which results to distorted image if stretched to width & height.

so we can zoom in as apple does, i tried it, but it affects the viewport thus the placement of the anchors in front of the image, we have to keep that in mind..

i can access a compatible device next week only :( but will follow the progress :)

@ofZach
Copy link
Collaborator Author

ofZach commented Aug 17, 2017

I found a simple solution was to add a small scale factor to the shader (where you set "textureCoordinate") -- I know it's not the most precise thing and we'd have to make it device independent but it's a very fast way to correct if you are prototyping...

to do it more precisely ofRectangle has a fit rect in rect function (ofRectangle::scaleTo) which can give you the largest rectangle in another to help get the zoom / crop coordinates.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Aug 17, 2017

AHHHHHH that's why it looked good, ugh I feel dumb.

I'm thinking doing things via the shader might be the way to go given the odd FBO issue and also to try keeping from piling more work on for the CPU; along those lines I think I have a possible solution(though of course it's impossible for me to test to see if it actually fixes anything with my device)

This was the change I tried.

         // scale is vec2(0.5);
        // 1.7 is aspect ratio - could pass that along as a uniform?
          vec2 fromCenter = vUv - scale;
           vec2 scaleFromCenter = fromCenter * vec2(1.7);
           vUv -= scaleFromCenter;

it seems to work and I do get a cropped image, I'm just not sure if it's too much or too little, will try with an FBO again to compare later(not near a charger and battery's almost dead).

What do you guys think? Was that more or less what you did too Zach?

@ofZach
Copy link
Collaborator Author

ofZach commented Aug 22, 2017

that's kind of what I did although I didn't scale from the center so this would make more sense (I just fudged a scale factor until things looked right)

@sortofsleepy
Copy link
Owner

sweet - I'll roll it in at some point, probably over the weekend to make it easier to test.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Aug 28, 2017

I know I'm way late on this but I finally got around to rolling the fix into the main codebase in develop. 😓

This should "hopefully" just work: basically since iPhones seem to be immune to this issue, the code now looks to see if you're on an iPad, then , if so, set's some values in the shader to try
and compensate for the distortion based on the bit of glsl above and also your device's aspect ratio.

Again - I have no iPad sized device to test this on so I have no idea how close it is or whether or not it actually fixes the issue, please let me know if this doesn't work and I can think about other ways to do this(now that I know why the FBO wasn't working before I may just go back to that)

@sortofsleepy
Copy link
Owner

Since I have yet to hear that this is not working from anyone, I'm gonna just go ahead and close this for now. Do feel free to re-open if the fix isn't good enough and we can revisit things.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 21, 2017

Bagh! Apparently not fixed, though with the Gold master version of IOS 11, it seems the cropping is just about right, the issue is now more on how to deal with scaling to match a device's viewport.

@sortofsleepy sortofsleepy reopened this Sep 21, 2017
@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 21, 2017

@HKBMedialab

Hi there!

I managed to find an iPad pro 10.5 as well here at work that had a version of 11.0 on it(since all the ipads at my work need to stay on their respective versions to test websites on so I wasn't able to test earlier) I just pushed a new branch called camera-correction that hopefully(*crosses fingers) addresses at least some of the issues you were seeing.

I didn't notice any noticeable distortion in my testing and the images were pretty close as far as I could tell between a blank project from the xcode wizard and what I'm seeing in the addon.

@sortofsleepy
Copy link
Owner

also - to anyone out there, I wouldn't mind help figuring out a good device independent way to solve this since there's a variety of devices out there; also because I'm a bit out of practice with manual image scaling/manipulation.

@HKBMedialab
Copy link

HKBMedialab commented Sep 22, 2017

Hey @sortofsleepy
thanks for the help. i am slightly confused about device locking. Do you suggest to not lock it?
Your camera-correction branch is working but i have some troubles concerning the rotation when not locked.
either i really don't get my head wrapped around what you are doing or you still stretch the video in line 254 in ARCam.mm :
cameraFbo.draw(0,0,ofGetWindowWidth(),ofGetWindowHeight());
this will rescale the image to wrong dimensions as far as i can think of.
if you zoom out a little and draw the fbo at its original size, you can see what is actually happening:
ofPushMatrix(); ofTranslate(0,0,-200); cameraFbo.draw(0,0); ofPopMatrix();

the FBO is still sort of 16/9 and you force it to the ipad. and at least for me, the dimensions are rotated wrong again. it is good at landscape left. then, there is no distortion as the fbodimensions are matching the cam. my keys are nice quadratic and the mugs really round. but in portrait the fbo width/height is flipped. you only could not see that if you draw the fbo like cameraFbo.draw(0,0,ofGetWindowWidth(),ofGetWindowHeight()); and i guess this causes the distortion...

Landscape Left is as far as i can see undestorted:
img_0086

Portrait is really much distorted:
img_0087
with cameraFbo.draw(0,0,ofGetWindowWidth(),ofGetWindowHeight()); you sort of over-undestort that again, i belive

@sortofsleepy
Copy link
Owner

@HKBMedialab

  • Apologies for the confusion 🙇 , I know I said something different a couple days ago but I was mistaken about how some of the ios SDK functions work. If you lock the device, the ARKit camera won't be able to update it's matrices properly.
  • cameraFbo.draw(0,0,ofGetWindowWidth(),ofGetWindowHeight()) exists because we need to fill the screen of the device using the width and height of the device at a minimum. The native capture resolution is 1280 / 720 which won't be able to fill the screen.
  • Apologies - I can't really see the difference with the code snippit you suggested 🙇

I honestly wasn't able to tell the difference between portrait and landscape unfortunately 😞 . Unfortunately I wasn't able to grab the iPad today, someone took it before I could get to it, but I'll keep my eyes open next week and try to grab it to test some more.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 22, 2017

@HKBMedialab ahhh thank you for the images, I do see the difference there. hmmm I'll need to think about this more then. What does it look like when it fills the screen?

@HKBMedialab
Copy link

Basically if i cout the dimensions for the cameraFBO after cam.scaleTo(screen,OF_ASPECT_RATIO_KEEP); i get 1536 x 864 which is a ratio of 1.7777 where i get 1536 x 2048 for the screen, which is a ratio of 1.333. if you stretch the cameraFBO to the screen in the ARCam::draw(), it leads to the distortion.

@sortofsleepy
Copy link
Owner

@HKBMedialab - just out of curiosity did you happen to pull down anything since you first pulled the camera-correction branch? I did make a few changes this morning(my morning) and I just want to be sure we're looking at the same code.

Also you can actually switch to develop now, it should be matched up with camera-correction

@HKBMedialab
Copy link

HKBMedialab commented Sep 22, 2017

ah, i pulled it about 4pm my time wich is about 3pm GMT i guess. (revision 1993f06)

sorry for not beeing clear about it. i try it again:
the other point is, that the FBO gets build here
cameraFbo.allocate(cam.getWidth(),cam.getHeight(), GL_RGBA);
as a Landscape image. even if your in portrait. you can see that, if you don't stretch it to the screen but draw it in its original size like
cameraFbo.draw(0,0) in line 254. (-> see images)

This is why it looks good in landscape then but not in portrait if you draw it in its original size. we could fix that if we get the ratio right i guess.
as i don't understand the shader yet, i would do it more hacky like just draw the FBO @2x in 0,0 minus some translation to center it and reallocate it if the device gets in portrait mode to
cameraFbo.allocate(cam.getHeight(),cam.getWidth(), GL_RGBA);
this would mean, that i would draw a slightly bigger FBO then the actual screensize...

@HKBMedialab
Copy link

HKBMedialab commented Sep 22, 2017

I think, its not really about the zoom factor in the shader but about cropping the image. the zoom is to fill the screen, but then, as the cam is 16/9 but the iPad is 4/3, we need also to crop it. Not sure if you where doing that in the shader but now there is no cropping.
or if not cropping it, just draw it wider than the screen and have a bit of it offscreen then.

@sortofsleepy
Copy link
Owner

ah - I think I kind of get what you're saying. hmmm. I'll need to think about this more later tonight (end of the workday here) but as a side note - the zoom factor is supposed to try and take care of that cropping. Drawing the image wider makes sense I know, but then the issue is how much to adjust the crop by and how to get the right value.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 23, 2017

@HKBMedialab
so eh , this may be unsolved until I can get my hands on an ipad again 😞 . Tried to figure things out on the ol iPhone but it's hard because there's no need to do any tweaking. But one final thought for the weekend... revision 1993f06 was already a little behind from the moment I asked you about how up to date you were 😛 . If you haven't already, switch to develop

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 23, 2017

so I may have figured something out which I just pushed to develop - I'll need to wait till I see can grab an Ipad but hopefully it's close? The issue I see now is potentially, the image might be too cropped in, also may need to figure out x/y coords to draw at(tried taking half of the image width but that didn't seem to work for some reason :/)

@HKBMedialab
Copy link

HKBMedialab commented Sep 24, 2017

Hey @sortofsleepy seems like you are on the right track. the image is not distorted anymore! well done! there are some minor things left though:

  • i agree, its seems a bit too much zoomed in and/or it seems not to be placed correctly, as things dont appear right on the exact spot where i place them but a bit higher.

  • if i am in landscape and then lay the device flat, the cam gets rotated half out of the display and i can see grey background and ARCam gives back UIInterfaceOrientationPortrait

  • UIInterfaceOrientationPortraitUpsideDown doesn't get triggered

i hope, this is not because of some interface settings i do wrong...

@HKBMedialab
Copy link

HKBMedialab commented Sep 24, 2017

and i had to revert near = 1.0; to near = 0.01; again in ARCam.mm Line 31 in order to see placed images.

@HKBMedialab
Copy link

@sortofsleepy if i center the camera image in ARCam.mm, i get the exact same image like in a blank ARKit project!
line 114 of ARCam.mm:

                      ofTranslate(0,-(cam.getWidth()*scaleVal-ofGetWindowHeight())/2,0);
                    cameraFbo.draw(0,0,cam.getHeight() * scaleVal,cam.getWidth() * scaleVal);
                    break;

Landscape should be ofTranslate(-(cam.getWidth()*scaleVal-ofGetWindowWidth())/2,0,0); then, but i am not really sure if this is the same as in a blank project as the blank does't rotate.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 24, 2017

sweet! Thanks for messing around with that! I'll keep that in mind when I'm able to test this again.

I think the goal ultimately is to just get things close enough, at least for now, just so the addon is more useable; distortion was the more concerning issue for me.

Also regarding near values, that was changed to try and fix z-fighting, instead of changing that value directly, you should mess around with setCameraNearClip or try tweaking the z value when an anchor is added. The anchor manager's addAnchor function accepts a z value or a ofVec3f

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 25, 2017

@HKBMedialab realized I forgot to address some points you made

if i am in landscape and then lay the device flat, the cam gets rotated half out of the display and i can see grey background and ARCam gives back UIInterfaceOrientationPortrait

  • hmmmm - I saw this behavior too but I'm confused, if the face up orientation change got triggered, it shouldn't spit back UIInterfaceOrientationPortrait, how did you figure that out?

UIInterfaceOrientationPortraitUpsideDown doesn't get triggered

  • this is expected unfortunately - it technically is detected and triggered, the device itself though, does not perform the orientation change(as far as I can tell) It appears to be either a SDK, project setup or ofxIOS issue, I'm not really sure at the moment. Given that, I'm not planning on supporting upside down portrait

Also, after playing around with a blank AR project a bit more - I think comparisons to that should be taken with a grain of salt, the default blank project does not appear to handle orientation changes, the default setting, somehow(I'm not sure how cause landscape right is checked as a supported orientation) appears to lock the screen to portrait like you discovered. When you change the settings to get rotation changes to happen, things are wacky.

@miflck
Copy link

miflck commented Sep 25, 2017

@sortofsleepy to be precise, i don't know what the device actually gives back. I just cout what case gets triggered in ARCam::draw(). Whenever I was in the UIInterfaceOrientationLandscapeLeft case and then laid it flat down, it switched to the UIInterfaceOrientationPortrait case. Only thing I cant explain is, why its not behaving like portrait-non-flat...

(btw, personally I think in many cases it makes totally sense to not handle orientationchanges with a interface rotation animation as this somehow brakes the continuity of the "Magic-Window"-experience. but of course in the best of all worlds the rotation should still be an option which you can choose based on conceptual or design reasons...)

@HKBMedialab
Copy link

@sortofsleepy sorry, i was logged in with my own account...

@sortofsleepy
Copy link
Owner

@HKBMedialab ah! That's interesting with it registering portrait in a landscape -> face down orientation transition 😖 (dang it apple !!)

I get what you're saying about not needing to see the rotation animation - that can fortunately be disabled outside of this library. I still need to manage these states though to get the right camera matrices though.

@sortofsleepy
Copy link
Owner

Since I don't know when, if ever, I'm gonna be able to get my hands on an iPad again at work, as a (hopefully) temporary measure, I just added setter functions for tweaking the x/y position of the image in develop.

The function is setCameraImagePosition and takes an x and y value. Theres also a getCameraImageBounds function to get the ofRectangle that has the scaled width/height values of the camera image.

@sortofsleepy
Copy link
Owner

sortofsleepy commented Sep 27, 2017

Ok! So some news - I'm getting moved to another office at my day job... which may make it more difficult for me to obtain an iPad to continue trying to figure out how to compensate for the image offsets. 😢 I tried spinning up a testing setup on the simulator with just a regular image but it's behaving a little strangely, probably due to different texture setup; I get the feeling it'll be one of those situations where it works fine on the simulator then totally blows up on an actual device. I'll keep at that when I have time.

I'll leave this open in the mean time and I also wrote up a quick explainer on current research as well as a little more detailed documentation on how to adjust the camera image if needed.

https://github.com/sortofsleepy/ofxARKit/wiki/Tweaking-the-Camera-image

@sortofsleepy
Copy link
Owner

sortofsleepy commented Nov 3, 2017

I may have just stumbled across a solution to this!
See:https://developer.apple.com/documentation/arkit/arframe/2923543-displaytransform
Not sure when that got added but I'll be looking to try that out this weekend.

Edit : drat! Got my hopes up, my memory is terrible heh - looks like I already tried this... hmmm

@sortofsleepy
Copy link
Owner

sortofsleepy commented Nov 9, 2017

I figured out what I was doing wrong in implementing the displaytransform function. Turns out I was a bit of an idiot, Started working towards rebuilding things to act more like the default metal example which hopefully will resolve this once and for all.

@ofZach
Copy link
Collaborator Author

ofZach commented Jan 31, 2018

I've been looking at this a bit on an iPad and I think the best thing I've found is to disable the perspective adjustment which seems awkward to me (I get video stretched in weird ways as I rotate the device):

void ARCam::draw(){

    // if we're on an iPad, things get weird. Adjust drawing based on viewport.
    if(false) { //needsPerspectiveAdjustment){

and to just scale the fbo when I draw it -- this is just scaling but scale and center might be better:

    cameraFbo.begin();
    processor->draw();
    cameraFbo.end();
    
    ofRectangle r(0,0,720, 1280);
    float ww = (float)ofGetWidth() / 720.0;
    r.scale(ww);
    cameraFbo.draw(r);

this gets me closer to the default metal look -- where as I rotate the device the video looks similar-ish...

maybe we can use some kind of internal fbo to scale and crop the video to the screen size?

@sortofsleepy
Copy link
Owner

@ofZach thanks for the note!

The needsPerspectiveAdjustment flag is really more to differentiate between iPhone and iPad at the moment since last I looked, iPhone's didn't really need any tweaking(though again, I was using a rather old 6S so I might be wrong with the newer models).

As to using an internal FBO - are you saying to use a second FBO to handle the crop/adjustment? There should already be an internal FBO being used.

Small side note : I think you might have an older version of the class? Looking at the first snippit you wrote - I'm pretty sure I wrote a very similar comment at one point. In any case, just a heads up that there have been a few updates for the camera class(see this for what's changed with how the image is drawn).

I had spent a couple of weekends last year trying to figure out good universal crop/adjustment math for this but didn't have much luck which lead to the above solution. Not the most elegant of course but figured it was at least a good intermediary method for the time being.

I think this chunk of code(maybe the entire thing) is due for a re-write soon anyways with the upcoming ability to render out different resolutions which I'm sure will complicate things more ha .😓

@ofZach
Copy link
Collaborator Author

ofZach commented Feb 1, 2018

thanks I'll test this out -- I am not always working of the latest master but I feel like the code is the same or similar. I'll double check.

my understanding of the issue is that all screens that have aspect ratios different than 1280x720 will need some love -- that includes the iphone 10, etc.

what I was seeing using your code (need to check the latest one) is that on ipad, as I rotate the phone 360 it gets weirder and weirder until it switches to the next orientation where it looks fine, but the act of rotating causes the image to sheer or warp...

I feel like what we should do generally is disregard orientation altogether and just draw the camera cropped to the screen aspect ratio (ie, take the largest inner rectangle of the cam which is the screens rectangle), which is is what it seems the metal app is doing.

I'll dig into this a little more...

@sortofsleepy
Copy link
Owner

Real quick since I'm getting ready to hop on a plane - that sounds like a plan - but wouldn't we still need to keep track of orientation to recalculate things since dimensions flip in landscape vs portrait?

(also quick note I forgot to mention - all the new code is on develop, not master)

@sortofsleepy
Copy link
Owner

sortofsleepy commented Jul 26, 2018

Ok! I know this has been open for awhile but I think I might have come up with a solution for this issue!

tl'dr

I've built up a Metal based camera renderer.

long form

It wasn't until recently that I discovered that it's possible to have Metal share texture information with OpenGL. With that information and the info that, well, it probably won't happen soon but along with the news that OpenGL will eventually get deprecated, I figured it couldn't hurt to start looking into a way to re-do the camera with Metal.

After going over the default XCode sample as well as an Apple provided example I've come up with a frankenstein renderer of sorts that renders the camera image with Metal, then transfers that information into an OpenGL texture.

Not only will it be somewhat future-proof but it should also

  • allow removal of all the extra code keeping track of orientation changes
  • be a little more color accurate (the current renderer does add a very slight orange tint to things)
  • can adjust itself based on the device you're using.

It's currently not integrated just yet pending further testing but it's far enough along that I figured I'd post about it. If you'd like to try it out, check out the camera-refactor branch. To see how to use it, I have another repo here which I'll add as an example later.

Gotchas

  • The Metal shader source is for some reason identified as just data by Xcode when starting a new project with the addon. Not sure why but the solution is to just change the file type to the default. This will fix the issue of not being able to find the Metal vertex shader.
  • Texture is flipped upside-down for some reason. I'll adjust the shader in the library but if you use it directly you'll have to make sure you flip the uvs on the plane you render the camera image to.
  • Not yet sure if theres a performance hit, I don't see anything noticeable but it's possible that there might be something there.(should be sorted!)
  • somewhat in between a minor and major issue - some work will need to be done to figure out what to do about orientation changes at start up; currently if your app is starting up and processes an orientation change before the image is available, it'll cause an error with the GPU and nothing will render.
  • Figuring out a good camera image size might still need a little more work. There shouldn't be any distortions as far as I can tell, but by default without any adjustments the camera image is REALLY zoomed in. I haven't figured out why; when using the renderer in a normal IOS app, the image looks fine. OpenGL has a max texture size of 4096 on IOS devices so it's not possible to simply multiply by a device-centric scale value as that will result in a error when transferring information to an OpenGL texture(on my 6s the height ends up being over 5000px hence the error). The current solution is to take the full resolution determined by the os and subtract the screen bounds from that. If someone has a better solution I'm all ears.
  CGRect screenBounds = [[UIScreen mainScreen] nativeBounds];
    
    // this is probably a more reasonable approach.
    width = self.currentDrawable.texture.width - screenBounds.size.width;
    height = self.currentDrawable.texture.height - screenBounds.size.height;

@aferriss
Copy link
Collaborator

Sounds great, nice work! I haven't looked at the branch yet but I wonder if the texture transfer from metal to openGL will also help as an example with #52 since the environment image is returned as a metal texture.

@sortofsleepy
Copy link
Owner

Yea I think it can help, the setup is a bit of a pain though, it might need to get integrated directly into a whatever will end up doing that.

Of note - one issue I've come across though with more testing - there seems to be an 20fps drop when this is running in conjunction with the other stuff. Trying to figure out why :/

@sortofsleepy
Copy link
Owner

🎉 figured it out - will push changes to both the branch and the example project. Turns out I didn't completely read the example hahahahah.

@sortofsleepy
Copy link
Owner

Ok metal based camera is included in develop now. I will get merged into master at some point baring any other issue. If this is still a problem please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants