Add exposure controls to UsdGeomCamera #3085
Conversation
Adds exposure:time, exposure:iso, exposure:fNumber, exposure:responsivity attributes and repurposes exposure attribute to be interpreted as exposure compensation. Adds UsdGeomCamera::GetExposureScale() method which allows users to calculate the imaging ratio (i.e. given a certain luminance on the sensor, what is the photometric exposure output) from these attributes. Also adds a unit test to test the above.
|
Filed as internal issue #USD-9673 |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- move factor of 100 in exposure equation to tie it to ISO - clarify that the exposure equation also takes into account the lens system - clarify how shutter:open/close, exposure:time, fStop and exposure:fNumber are related
pxr/usd/usdGeom/schema.usda
Outdated
| The combination of these parameters gives an exposure scaling factor (also | ||
| referred to as the __imaging ratio__) that converts from incident luminance | ||
| passing through the lens system and reaching the sensor, in __nits__, to an | ||
| output signal based on photometric exposure in __lux-seconds__. |
There was a problem hiding this comment.
For clarity, is the "imaging ratio" the factor that converts the luminance in nits=cd/m^2 (=lux/radians^2, I think) into exposure in lux-seconds (so [per-wavelength perceptually adjusted] energy per area) of the film? -So "imaging ratio" has unit radians^2 second(??)? Or is this the factor that converts it all the way from luminance to actual the unit-less floating point value that I want to write into some image file, e.g., OpenEXR? So it's unit is simply 1/nits?
Since the factor also includes the iso, it seems to me that it is the latter. In that case, I guess exposureResponsivity should have unit "1/(nits * 100ISO)" - if ISO were an actual unit.
Minor nitpick: "passing through" sounds like you only look at the light that makes it through, say, the coating of a lens. Would "Enter the lens system" be more correct? And where would you author what fraction of light doesn't make it through the lens system?
Note: It might be nice to remind the reader that nits = cd/m^2 in SI units. Bonus: lux and nits should actually be related by radians^2. Or even better, maybe say that ignoring units, the "imaging ratio" is the value of the output signal you get if you had nits luminance entering the lens or equivalently a large Lambertian radiator far from the lens radiating with f lux (where f should be 1/area of sphere, I think, I might be missing some factor here).
There was a problem hiding this comment.
Minor nitpick: "passing through" sounds like you only look at the light that makes it through, say, the coating of a lens. Would "Enter the lens system" be more correct? And where would you author what fraction of light doesn't make it through the lens system?
This is the light that gets all the way through to the sensor. The fraction of light that doesn't make it due to being absorbed (plus other effects) is 1-responsivity.
There was a problem hiding this comment.
Thanks.
Reading "image ratio", I want to know what the numerator and the denominator is. Currently, it says "factor that converts from [1] to [2]". So I should be able to read of the denominator from [1] and numerator from [2]. [1] ends in "reaching the sensor". So if I read this literally, "image ratio" does not account for anything that happens in front of the sensor such as light being absorbed by lens coating. But you are just saying that such an affect would be accounted for by the responsivity.
I know that an expert would probably not read it this way. But I think the wording in the usd schema should be unambiguous. Breaking up that sentence should achieve this.
There was a problem hiding this comment.
I'm sorry I'm not following, when you say "break up the sentence" what do you mean exactly?
There was a problem hiding this comment.
I was thinking of first defining the "imaging ratio" by unambiguously saying what it is the ratio of or what it is converting from and to. And then listing intermediate steps or contributing factors.
"from incident luminance passing through the lens system and reaching the sensor" can be interpreted in multiple ways.
There was a problem hiding this comment.
OK I've added the units of the imaging ratio (sr.s) and noted that nits are cd/m^2.
There was a problem hiding this comment.
@unhyperbolic pushed another update that fixes my bad dimensional analysis and more explicitly clarifies the conversion and units involved. Let me know what you think.
There was a problem hiding this comment.
@unhyperbolic polite bump here - does this address your concerns?
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run |
|
Apologies for getting sidetracked; Anders had requested I post my edits back here for NVidia to look at: From PR description: Pixar schema feedback, UsdImaging 2.0 implementation. I tried to pull the merge artifacts out, since some of these files are out of date w.r.t. top of tree, apologies if I missed anything. The header license text changed everywhere. Schema feedback: there were some 80 characters changes but the only substantial change was renaming "GetExposureScale" to "ComputeLinearExposureScale". "Compute" because it's idiomatic to call it compute when it's doing a computation; Get is only for data access. "Linear" because exposure elsewhere in USD is log2-encoded, and it seemed like a nice clarification. I added UsdImagingStageSceneIndex support as well as fixed some invalidation issues in UsdImagingDelegate support. Further discussion topics: I'm worried about the token swap in usdImaging: hydra exposure -> usd computed exposure, hydra exposureCompensation -> usd exposure, as a source of future bugs. Other potential avenues of dealing with this would be:
Let me know what you think of those three options. We're happy with the math and the schema (with the one function rename), and just needed to iterate on the usdImaging code a bit. There were some additional internal complications that are resolved as well. |
|
I like option 1 - updating to UsdGeomCamera_1 and changing |
|
/AzurePipelines run |
|
Pull request contains merge conflicts. |
Usd geom camera exposure
# Conflicts: # pxr/usd/usdGeom/wrapTokens.cpp
pmolodo
force-pushed
the
usdGeomCamera-exposure
branch
from
8faf7ff to
43b89cd
Compare
...by making exposure hydra attribute map to exposure USD attribute, and exposureScale map to ComputeLinearExposureScale. This means that existing render delegates that already query exposure will not get the more advanced behavior "for free", and will need to be updated to query exposureScale instead, but should lessen future confusion and potential bugs.
|
Hi - so despite initially leaning toward option 1, after @nvmkuruc pointed out that versioning the schema would make all uses of the So we now think Option 2 would be best:
The need to update render delegates seems an acceptable cost, compared to the much larger scale changes we'd need with the schema version change. So I've pushed some commits which make that switch (as well as merge in |
|
|
||
| _inputs.resize(inputNames.size()); | ||
| UsdPrim prim = _usdCamera.GetPrim(); | ||
| for (TfToken inputName : inputNames) { |
There was a problem hiding this comment.
@pmolodo If possible, avoid a copy of TfToken as it increases the ref count-- It should be safe to use const TfToken& or const auto& here.
There was a problem hiding this comment.
|
|
||
| static UsdAttribute | ||
| _CreateExposureIsoAttr(UsdGeomCamera &self, | ||
| object defaultVal, bool writeSparsely) { |
There was a problem hiding this comment.
Is this spacing offset intentional?
There was a problem hiding this comment.
Not sure - but every _CreateXXX function in that file has the same spacing. Should probably fix all or none.
CMakeLists.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.14) | |||
| cmake_minimum_required(VERSION 3.15) | |||
There was a problem hiding this comment.
@pmolodo Was the cmake bump intentional?
There was a problem hiding this comment.
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pmolodo
force-pushed
the
usdGeomCamera-exposure
branch
from
86c6dcd to
0ae21ac
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Made a follow-up PR, that adds Storm support: To see just the changes vs THIS PR, see here: |
| @@ -265,7 +265,7 @@ UsdImagingCameraAdapter::Get(UsdPrim const& prim, | |||
| return vExposureResponsivity; | |||
| } else if (key == HdCameraTokens->exposureScale) { | |||
| // The computed linear exposure multiplier ratio | |||
| return VtValue(log2(cam.ComputeLinearExposureScale(time))); | |||
| return VtValue(cam.ComputeLinearExposureScale(time)); | |||
There was a problem hiding this comment.
This had been log2 throughout, to match the encoding of "exposure". Is the loss of precision such that this is necessary for you all? If so: (1) let's make the hydra token "linearExposureScale" and (2) let's update dataSourceCamera.cpp correspondingly.
There was a problem hiding this comment.
This was just log2 so that it backdoored into the existing exposure field (since that's log2). Since we're not doing that hijack any more this wants to be linear. @pmolodo can you update the token per Tom's comment?
There was a problem hiding this comment.
The thinking behind linear wasn't avoiding precision loss, so much as simplicity and consistency:
- the value is most easily expressed and calculated in linear space (since of the 5 component attributes, only 1 is in log2 space)
- the value will generally be used programmatically in linear space (ie, as a pixel value multiplier)
- as @anderslanglands points out, we formerly only converted to log2 where necessary for shoe-horning back into the exposure attribute. Now that we're making "exposure" and "exposureScale" completely separate attributes, and not worrying about existing implementations picking up the new behavior / attributes "for free", we have the opportunity to make ALL usages of exposureScale in linear space.
Regardless, I totally agree with you that the possibility for confusion is high - so, in addition to making a commit where the token names are changed to "linearExposureScale", I also added a commit where ALL references to "exposureScale" in the code are changed to "linearExposureScale", for clarity. (You can leave that commit out if you feel it's too verbose, though!)
...for both HdCameraTokens and HdCameraSchemaTokens
...to make it clear this value is a flat multiplier, not a log2 exponent
pmolodo
force-pushed
the
usdGeomCamera-exposure
branch
from
33874a4 to
07496b0
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This PR adds exposure controls to UsdGeomCamera, allowing to specify brightness as with a physical camera. The calculated exposure value is fed to
HdCamera::GetExposure()viaUsdImagingCameraAdapter. This means that:exposureattribute continue to work as beforeHdCamera::GetExposure()get the exposure calculation automatically if the new exposure attibutes are changed from their defaults.This is currently draft to start discussion as per @meshula
Description of Change(s)
Adds exposure:time, exposure:iso, exposure:fNumber, exposure:responsivity attributes and repurposes exposure attribute to be interpreted as exposure compensation. Adds UsdGeomCamera::ComputeLinearExposureScale() method which allows users to calculate the imaging ratio (i.e. given a certain luminance on the sensor, what is the photometric exposure output) from these attributes.
Also adds a unit test to test the above.
Adds tokens
exposureTime,exposureIso,exposureFNumber,exposureResponsivity,exposureCompensationtoHdCamera. AddsHdCamera::GetLinearExposureScale()for delegates to be able to use the new controls with a single function call.