display.c's crop function (the VLC video crop effect) is broken when videos have a metadata orientation value (directX behavior also incorrect on non-metadata rotated video)
tldr;
DirectX metadata orientation rotation rendering with crop is very inconsistent use an openGL method for it to make any sense. The downstream crop/aspect ratio rendering functions should likely handle rotated videos better as the arguments passed to them for a rotated video are not-intuitive.
The attached patch is against the 3.x branch and while it seems to work in all cases tested (as long as a DX renderer is not used) I am not sure it should be the permanent code. I also have no interest in further working on this issue or revising the patch as I have an easier work around. Due to these two things I am putting the patch in this bug ticket rather than properly submitted.
I have submitted two patches properly to the mailing list for fixes metadata orientation in cropadd.
Background of the Problem / Odd VLC Behavior
Loading a 1920x1080 video with 90 degree metadata rotation (so now portrait) instantly has the bottom cut off (so it is now say 1800x1080). This is before any crop is applied and is not running through the crop filter (this it turns out is due to DX).
Loading a 1080x1920 video with 90 degree rotation (now landscape) instantly at the top cut off (so now 950x1920) (again DX related).
The 1800 and 950 are random I didn't spend the time to see how many pixels exactly are cut off.
Loading a non rotated video that is 1920x1080 then using the video effect transform to rotate it 90 degrees does not have the bottom clipped like the rotate meta data flag does.
Cropping a non rotated video that is 1920x1080 that has a 90 degree transform effect goes wrong. If you were to enter 530 for left and right crop you should just get a 20 pixel wide line, you get something much wider. Entering a left crop of 350 does not crop nearly 350 pixels over. You need to multiply each of those numbers by 16/9 (1.77) to get the correct crop amount applied. Otherwise crop seems to work.
Cropping a video that has rotation metadata applied is a disaster. Enter a value of 500 in any of the boxes, for example, to see distortion and odd cropping. Enter a value of the width or the height / 2 on the correct side and it will just go blank. One less than that and it will still display something.
These two may be the same issue: #25714 #25593 (closed) although what is odd for me, is running Win10 a rotated video has the crop/cutoff applied if DirectX 11 is used but not if DirectX 9 or openGL are used (I set the preference and restarted vlc to make sure it took effect). It also does not happen on the same video if I transpose it and remove the rotate flag ;)?.
Finally Both DirectX 9 and DirectX 11(at least on my Win10 machine) are heavily screwed up when it comes to crop (and also behave differently). This took many more hours to figure out because they behave very erratically and not in a predictable fashion (as far as I can tell) with the rotation data passed. Once I switched to the OpenGL renderer I was able to get consistent behavior.
Summary of the Existing Code and File Details
For those who might not know this specific code:
This is for videos with metadata rotation this is where metadata has it rotate 90/180/270 when viewed, frequently found on videos shot with phones. For an example with phones, the sensor may always record so the part near the speaker on the phone is always the bottom of the video. It may always record at 1920x1080 no matter what orientation you hold the phone. To make the video show right side up the metadata rotation is put on the file. The true file may always be 1920x1080 but use rotation to have it displayed 1080x1920. I would describe the files native dimensions as the 'true' dimensions and the dimensions considering rotation to be the rendered dimensions. Dimensions are represented in code as the total width/height and then an x/y offset (using these 4 things you can get any rectangular crop you want).
The crop tool, in effect, is implemented using display.c's vout_ManageDisplay. In there is where the crop effect is calculated. The section of that function dealing with when crop changes is contained in the if on "osys->ch_crop". There are two primary variables in play vd->source and osys->source/osys->crop. osys is the original video data. The crop is initially set to the user requested crop but depending on what options (ie crop to an aspect) we may update osys->crop with new numbers. For the most part osys is just used to read the existing/original data. vd->source is our video out display. This is most of what we configure the visible dimensions and offset for the final video view. This is used not just for cropping but also for determining the aspect ratio to maintain.
The function itself does primarily does two things: It calls vout_display_Control(vd, VOUT_DISPLAY_CHANGE_SOURCE_CROP) and sets i_visible_width/i_visible_height and i_x_offset/i_y_offset on the vout_display_t (vd) video format args. The vout_display_Control this takes, in theory, the new display offsets and width/height and fixes the aspect ratio it should maintain once cropped.
Patch Details
I have attached a patch that fixes videos that have orientation metadata and are then cropped. It works reliably in all the rotations I tried with all the crops I tried AS LONG AS you do not use the directX renderer. The OpenGL renderer is completely broken for the rotate / transform geometry options in effects nothing happens no matter the value I set for either even with corp off) so I can't tell if this patch works with rotated videos that way that have metadata and then are cropped. What I can say is using the existing crop function on a non-metadata video that you also use the rotate filter on is broken with DirectX.
The 'Fix' is low quality code but is the end of the time I want to spend on this issue (as I don't actually need this feature). It also doesn't work with the DirectX engines but that is likely more due to bugs in the VLC directX rotation handling then anything else (although maybe if that is fixed then a more basic solution would work for dX and the openGL renderer has a slightly different issue.
Sadly for orientation changed videos it is a bit odd, to get vout_display_Control to function properly you provide the correct render offset (so to crop from the top as rotated you do set the Y offset amount), but you swap the render width and render height passed to that function to get it to work. You still compute them using the correct render way (so render_width = actual width - left crop - right crop; still). The second thing it does is set the i_visible_width/i_visible_height and i_x_offset/i_y_offset on the values for the actual cropping that occurs. This is where the code that implements those values downstream needs very odd (but computable) values to operate correctly. In a non rotated video those 4 variables (width/height/x/y offset) are just as expected and crop as expected. To get crop to work with rotated videos I found you need to multiply your desired width by the original width/height. For an example if you have a video that is 1920x1080 (and not rotated) and had wanted render crop values values: top=40, bottom=50, left=15,right=30. You would return visible width=1920-left-right and height=1080-top-bottom so end up with: width=1875 height=990, your offsets are x=left(15), y=top(40). Now take that same video that has true size 1920x1080 with a rotate of 90 on it to render as 1080x1920 and we want to get the same cropped frame even though we are now looking at it in portrait. First we need to rotate all our crop values 90 degrees so top=left, right=top, bottom=right, left=bottom top=15, bottom=30, left=50, right=40. Our final dimensions should still be rendered as 990x1875 x=50,y=15 when displayed. However the value we return from the function are off.
width = render_width*true_width/true_height. width = 990 * 1920/1080 = 1760
height = render_height * true_height/true_width; height = 1875 * 1080/1920 = 1054; Finally both offset_x and offset_y need to be multiplied by those same ratio values: x_offset = 50 * 1920/1080 = 89 y_offset = 15 * 1080/1920 = 8
In terms of the values when dealing with rotated videos the user interacts with it in the rendered rotated state vs the native state being used in most of the code. When the user gives input based on the rendered state (say crop dimensions) there are two options for dealing with it:
- Rotate the user input to match the true video dimensions (the cropadd modification patches do this)
- Swap the real dimensions to match the rendered perspective. Making sure to swap them back to the real dimensions if ever required.
For the changes in this function if everything worked correctly we would likely not need to ever swap back to real dimensions (when dealing with viewport / rendered outputs). I did go with the second method. While the first method may seem like the initially less complex a few things were downsides of it: debugging/tracing is a bit trickier, if you are thinking about how the video is displayed that is relative, so you are constantly trying to rotate values (and make sure you are thinking about the right direction for rotation). The inputs are given in 4 offsets converted to width/height soon after. To rotate we need to use the initial 4 offsets and we also need to rebalance the negative/positive values (as the function uses negatives to have it compute the width value vs treating it as an absolute width). We also need to output to render functions you have to rotate back to render dimensions when you do. Finally we set the crop values (which are in render perspective) incase they were also computed, so these would need to be rotated back too. There are helpers to handle rotation, but the first method seemed like more rotations and combined with the harder time to debug (width/height mean the opposite for a relative rotated video with method 1). In terms of debugging consider having two videos that render the exact same, say 1080x1920. One is actually recorded at 1920x1080 with a rotate metadata. If you do use method 1 tracing all the computations through the function will be completely different for the two videos (so width/height/x/y reversed). With method 2 you simply swap the real values at the start of the function and all the rest of the computations are exactly the same.
The attached patch looks like it changes more logic than it actually does. Everywhere in the crop function/segment that originally referenced the original dimensions (osys x_offset/y_offset/display_width/display_height ie: osys->source->x_offset) where replaced with local variables orig_*. This is so that if the video is rotated we can swap all those variables. You can find the actual logic changes beyond that by looking for the if ORIENT_IS_SWAP(orientation) existing function that determines if we should be swapping.
I did submit two patches to the mailing list to fix the separate cropadd module (far easier) #25053 (closed) in master and 3.x (as there is a new crop module in master I am not sure if old one will still be used).0001-Fixes-display-cropping-with-orientation-metadata-for-3.0.x.patch