Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Autoexposure example restoration#728

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
devshgraphicsprogramming wants to merge47 commits intomaster
base:master
Choose a base branch
Loading
fromautoexposue_ex

Conversation

@devshgraphicsprogramming
Copy link
Member

Description

Testing

TODO list:

Comment on lines 23 to 61
template <typename T>
Tmorton2d_mask(uint16_t _n)
{
conststatic uint64_t mask[5] =
{
0x5555555555555555ull,
0x3333333333333333ull,
0x0F0F0F0F0F0F0F0Full,
0x00FF00FF00FF00FFull,
0x0000FFFF0000FFFFull
};
return (T)mask[_n];
}

template <typename T>
Tmorton3d_mask(uint16_t _n)
{
conststatic uint64_t mask[5] =
{
0x1249249249249249ull,
0x10C30C30C30C30C3ull,
0x010F00F00F00F00Full,
0x001F0000FF0000FFull,
0x001F00000000FFFFull
};
return (T)mask[_n];
}
template <typename T>
Tmorton4d_mask(uint16_t _n)
{
conststatic uint64_t mask[4] =
{
0x1111111111111111ull,
0x0303030303030303ull,
0x000F000F000F000Full,
0x000000FF000000FFull
};
return (T)mask[_n];
}
Copy link
MemberAuthor

@devshgraphicsprogrammingdevshgraphicsprogrammingAug 16, 2024•
edited
Loading

Choose a reason for hiding this comment

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

couldn't you make them into

namespacemorton{namespaceimpl{template<uint16_t Dims,typename T>structmask;// now the partial specializations}}

with the masks asNBL_CONSTEXPR member variables

This way you can have

template<uint16_t Dims,typename T,uint16_t BitDepth>enable_if_t<is_scalar_v<T>&&is_integral_v<T>,T>decode(T x){static_assert(BitDepth <=sizeof(T)*8);   x = x & mask<Dims,T>::value[0];      .. rest ofif-else statements}

Choose a reason for hiding this comment

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

@Fletterio will take over

Comment on lines +268 to +278
template<typename T, uint32_t bitDepth =sizeof(T) * 8u>
Tmorton2d_decode_x(T _morton) {return impl::morton2d_decode<T, bitDepth>(_morton); }
template<typename T, uint32_t bitDepth =sizeof(T) * 8u>
Tmorton2d_decode_y(T _morton) {return impl::morton2d_decode<T, bitDepth>(_morton >>1); }

template<typename T, uint32_t bitDepth =sizeof(T) * 8u>
Tmorton2d_encode(T x, T y) {return impl::separate_bits_2d<T, bitDepth>(x) | (impl::separate_bits_2d<T, bitDepth>(y) <<1); }
template<typename T, uint32_t bitDepth =sizeof(T) * 8u>
Tmorton3d_encode(T x, T y, T z) {return impl::separate_bits_3d<T, bitDepth>(x) | (impl::separate_bits_3d<T, bitDepth>(y) <<1) | (impl::separate_bits_3d<T, bitDepth>(z) <<2); }
template<typename T, uint32_t bitDepth =sizeof(T) * 8u>
Tmorton4d_encode(T x, T y, T z, T w) {return impl::separate_bits_4d<T, bitDepth>(x) | (impl::separate_bits_4d<T, bitDepth>(y) <<1) | (impl::separate_bits_4d<T, bitDepth>(z) <<2) | (impl::separate_bits_4d<T, bitDepth>(w) <<3); }

Choose a reason for hiding this comment

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

imho these should betemplate<uint16_t Dims, typename T, uint16_t BitDepth=sizeof(T)*8>

because that way you only need to write the encode and decode once

template<uint16_t Dims,typename T,uint16_t BitDepth=sizeof(T)*8>vector<T,Dims>decode(const T _morton){    vector<T,Dims> retval;for (uint16_t i=0; i<Dims; i++)       retval[i] = impl::decode<Dims,T,BitDepth>(_morton>>i);return retval;}template<uint16_t Dims,typename T,uint16_t BitDepth=sizeof(T)*8>Tencode(const vector<T,Dims> coord){    T retval = impl::separate_bits<Dims,T,BitDepth>(coord[0]);for (uint16_t i=1; i<Dims; i++)       retval |= impl::separate_bits<Dims,T,BitDepth>(coord[1])<<i;return retval;}

Choose a reason for hiding this comment

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

@Fletterio will take over

Comment on lines 40 to 41
retval.minLuma = lumaMinimum;
retval.maxLuma = lumaMaximum;

Choose a reason for hiding this comment

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

typo, you've set the min and max equal to each other

Choose a reason for hiding this comment

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

P.S. its also more useful to take a precomputedminLumaLog2 andlumaLog2Range (diff between log of max and log of min)

Choose a reason for hiding this comment

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

still outstanding for the geom meter

Comment on lines 64 to 105
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_instruction(spv::OpAtomicIAdd)]]
enable_if_t<is_same_v<T,uint32_t> || is_same_v<T,int32_t>, T>atomicIAdd([[vk::ext_reference]] T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T, typename Ptr_T>// DXC Workaround
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_instruction(spv::OpAtomicIAdd)]]
enable_if_t<is_spirv_type_v<Ptr_T> && (is_same_v<T,uint32_t> || is_same_v<T,int32_t>), T>atomicIAdd(Ptr_T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T>// integers operate on 2s complement so same op for signed and unsigned
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_capability(spv::CapabilityInt64Atomics)]]
[[vk::ext_instruction(spv::OpAtomicIAdd)]]
enable_if_t<is_same_v<T,uint64_t> || is_same_v<T,int64_t>, T>atomicIAdd([[vk::ext_reference]] T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T, typename Ptr_T>// DXC Workaround
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_capability(spv::CapabilityInt64Atomics)]]
[[vk::ext_instruction(spv::OpAtomicIAdd)]]
enable_if_t<is_spirv_type_v<Ptr_T> && (is_same_v<T,uint64_t> || is_same_v<T,int64_t>), T>atomicIAdd(Ptr_T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T>// integers operate on 2s complement so same op for signed and unsigned
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_instruction(spv::OpAtomicISub)]]
enable_if_t<is_same_v<T,uint32_t> || is_same_v<T,int32_t>, T>atomicISub([[vk::ext_reference]] T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T, typename Ptr_T>// DXC Workaround
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_instruction(spv::OpAtomicISub)]]
enable_if_t<is_spirv_type_v<Ptr_T> && (is_same_v<T,uint32_t> || is_same_v<T,int32_t>), T>atomicISub(Ptr_T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T>// integers operate on 2s complement so same op for signed and unsigned
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_capability(spv::CapabilityInt64Atomics)]]
[[vk::ext_instruction(spv::OpAtomicISub)]]
enable_if_t<is_same_v<T,uint64_t> || is_same_v<T,int64_t>, T>atomicISub([[vk::ext_reference]] T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

template<typename T, typename Ptr_T>// DXC Workaround
[[vk::ext_capability(spv::CapabilityPhysicalStorageBufferAddresses)]]
[[vk::ext_capability(spv::CapabilityInt64Atomics)]]
[[vk::ext_instruction(spv::OpAtomicISub)]]
enable_if_t<is_spirv_type_v<Ptr_T> && (is_same_v<T,uint64_t> || is_same_v<T,int64_t>), T>atomicISub(Ptr_T ptr, uint32_t memoryScope, uint32_t memorySemantics, T value);

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

you can now even check forPtr_T being a pointer

NBL_CONSTEXPR_STATIC_INLINEbool is_pointer_v = is_pointer<T>::value;

template<typename T = float32_t>
struct Reinhard
{
using float_t = enable_if_t<is_floating_point<T>::value, T>;

Choose a reason for hiding this comment

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

with new concepts, the constraint should be applied viaNBL_PRIMARY_REQUIRES

LIST_BUILTIN_RESOURCE(NBL_RESOURCES_TO_EMBED"hlsl/luma_meter/common.hlsl")
LIST_BUILTIN_RESOURCE(NBL_RESOURCES_TO_EMBED"hlsl/luma_meter/luma_meter.hlsl")
# tonemapper
LIST_BUILTIN_RESOURCE(NBL_RESOURCES_TO_EMBED"hlsl/tonemapper/operators.hlsl")

Choose a reason for hiding this comment

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

I'd createtonemapper/operators/reinhardt andtonemapper/operators/aces instead of slapping everything into one file

LIST_BUILTIN_RESOURCE(NBL_RESOURCES_TO_EMBED"hlsl/bda/bda_accessor.hlsl")
# luma metering
LIST_BUILTIN_RESOURCE(NBL_RESOURCES_TO_EMBED"hlsl/luma_meter/common.hlsl")
LIST_BUILTIN_RESOURCE(NBL_RESOURCES_TO_EMBED"hlsl/luma_meter/luma_meter.hlsl")

Choose a reason for hiding this comment

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

similarly,luma_meter/luma_meter is a tautology, a file each likeluma_meter/histogram andluma_meter/geom_avg would be nice

template<uint32_t GroupSize, typename ValueAccessor, typename SharedAccessor, typename TexAccessor>
struct geom_meter {
using float_t = typename SharedAccessor::type;
using float_t2 = typename conditional<is_same_v<float_t, float32_t>, float32_t2, float16_t2>::type;

Choose a reason for hiding this comment

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

even if doing color computation infloat16_t this doesn't free you from doing texture coordinate calc infloat32_t

}

float_t sampleCount;
float_t2 lumaMinMax;

Choose a reason for hiding this comment

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

don't do weird things we used to do in GLSL (due to no scalar layout), have a separate variable for min and max

Choose a reason for hiding this comment

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

also you should have the min and max precomputed withlog2 already applied

Comment on lines +138 to +141
return (luma / (1 << fixedPointBitsLeft)) / sampleCount;
}

float_t sampleCount;

Choose a reason for hiding this comment

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

you want to compute and store the reciprocal ofsampleCount and the1<<fixedPointBitsLeft

Choose a reason for hiding this comment

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

that was the purpose of thercpFirstPassWGCount variable in the old GLSL

Comment on lines +135 to +136
uint32_t3 workGroupCount = glsl::gl_NumWorkGroups();
uint32_t fixedPointBitsLeft =32 -uint32_t(ceil(log2(workGroupCount.x * workGroupCount.y * workGroupCount.z))) + glsl::gl_SubgroupSizeLog2();

Choose a reason for hiding this comment

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

you're supposed to normalize by the number of samples you took during the sampling step, yourworkGroupCount here is NOT that value, its the number of workgroups you're exposing with

Choose a reason for hiding this comment

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

You must precompute thefixedPointsBitsLeft in thecreate method (and it needs to know how many invocations you'll be running the sample step)

Comment on lines 45 to 51
float_tcomputeLumaLog2(
NBL_CONST_REF_ARG(MeteringWindow) window,
NBL_REF_ARG(TexAccessor) tex,
float_t2 shiftedCoord
)
{
float_t2 uvPos = shiftedCoord * window.meteringWindowScale + window.meteringWindowOffset;

Choose a reason for hiding this comment

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

precompute a scale and offset from the Metering Window + the number of workgroups + the workgroup size to apply to auint16_t2 unnormalized coordinate

right now you have waaay too many variables:

  • tile Offset
  • viewport size
  • metering window
    which are being manipulated per-pixel

NBL_REF_ARG(ValueAccessor) val,
NBL_REF_ARG(TexAccessor) tex,
NBL_REF_ARG(SharedAccessor) sdata,
float_t2 tileOffset,

Choose a reason for hiding this comment

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

why is tile Offset being provided from the outside? its a byproduct of your workgroupID, and workgroupSize-1 decoded as morton +1 in each dimension

Comment on lines 55 to 57
luma =clamp(luma, lumaMinMax.x, lumaMinMax.y);

returnmax(log2(luma),log2(lumaMinMax.x));

Choose a reason for hiding this comment

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

whymax you already clamped!

Choose a reason for hiding this comment

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

btw if you have thelog2 already precomputed then you can do

return min(spirv::nMax(log2(luma),lumaMinLog2),lumaMaxLog2);

nMin is a special SPIR-V version of min that will return the other operand if another is NaN (which happens on log of negative value or 0)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

Comment on lines 102 to 104
float_t luma =0.0f;
float_t2 shiftedCoord = (tileOffset + (float32_t2)(coord)) / viewportSize;
luma =computeLumaLog2(window, tex, shiftedCoord);

Choose a reason for hiding this comment

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

luma should be calledlumaLog2

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

luma =computeLumaLog2(window, tex, shiftedCoord);
float_t lumaSum =reduction(luma, sdata);

if (tid == GroupSize -1) {

Choose a reason for hiding this comment

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

its somewhat semantically cleaner to pick the first, instead of last, esp since its a reduction you performed before

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed


voiduploadFloat(
NBL_REF_ARG(ValueAccessor) val_accessor,
uint32_t index,

Choose a reason for hiding this comment

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

don't give bad names to variables, this needs to be calledworkGroupIndex

Choose a reason for hiding this comment

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

this variable shouldn't even exist, because the MEAN meter didn't output to different address per X Y workgroup coordinate

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed


if (tid == GroupSize -1) {
uint32_t3 workgroupCount = glsl::gl_NumWorkGroups();
uint32_t workgroupIndex = (workgroupCount.x * workgroupCount.y * workgroupCount.z) /64;

Choose a reason for hiding this comment

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

you're computing the wrong thing, every workgroup gets the same index 🤦

Also the original code was touching the same address with every for the MEAN meter mode

float_t rangeLog2
)
{
uint32_t3 workGroupCount = glsl::gl_NumWorkGroups();

Choose a reason for hiding this comment

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

take the workgroup count and workgroup XYZ coordinate (or workgroup index) from outside (as function arguments) otherwise in the presence of solutions such as virtual workgroups or persistent threads, this whole thing will fall apart

float_t lumaSum =reduction(luma, sdata);

if (tid == GroupSize -1) {
uint32_t3 workgroupCount = glsl::gl_NumWorkGroups();

Choose a reason for hiding this comment

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

take the workgroup count and workgroup XYZ coordinate (or workgroup index) from outside (as function arguments) otherwise in the presence of solutions such as virtual workgroups or persistent threads, this whole thing will fall apart

Comment on lines +64 to +65
float_t minLog2,
float_t rangeLog2

Choose a reason for hiding this comment

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

should already be precomputed as members

Comment on lines +69 to +71
uint32_t fixedPointBitsLeft =32 -uint32_t(ceil(log2(workGroupCount.x * workGroupCount.y * workGroupCount.z))) + glsl::gl_SubgroupSizeLog2();

uint32_t lumaSumBitPattern =uint32_t(clamp((val - minLog2) * rangeLog2,0.f,float32_t((1 << fixedPointBitsLeft) -1)));

Choose a reason for hiding this comment

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

lets write some docs for this....

Theval was produced by a workgroup reduction is performed of values in the[MinLog2,MaxLog2] range

Which makes thescaledLogLuma (the variable that should hold(val-minLog2)*rangeLog2) is between 0 and WorkGroupSize

This value is atomic added by N workgroups

You now want to represent it in Fixed Point during the atomic add, but not be vulnerable to overflow, this means the worst case is adding N times WorkGroupSize.

This means that we need to multiply the by(2^32-1)/N precomputed as a float or if you must round upN to PoT and see how many bits are left (512 workgroups, means 9 bits, so 23 are left). To avoid rounding precision errors, the PoT method is chosen.

I have no clue where you're getting+SubgroupSizeLog2 from.

Also the value of(1<<fixedPointBitsLeft)-1 must be precomputed increate and stored as a member

IT should be as easy as

constuint32_t scaledLumaLog2BitPattern =uint32_t((val-lumaMinLog2)*maxIncrement_over_lumaRangeLog2+float_t(0.5));

wheremaxIncrement = (0x1u<<(32u-uint32_t(ceil(log2(WorkGroupCount*WorkGroupSize)))))-1;


uint32_t lumaSumBitPattern =uint32_t(clamp((val - minLog2) * rangeLog2,0.f,float32_t((1 << fixedPointBitsLeft) -1)));

val_accessor.atomicAdd(index & ((1 << glsl::gl_SubgroupSizeLog2()) -1), lumaSumBitPattern);

Choose a reason for hiding this comment

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

no, always the same address should be added to, if you wanted to stagger, then you should stagger based on modulo of the workgroup index

Comment on lines +83 to +84
float_t luma = (float_t)val_accessor.get(index & ((1 << glsl::gl_SubgroupSizeLog2()) -1));
return luma / rangeLog2 + minLog2;
Copy link
MemberAuthor

@devshgraphicsprogrammingdevshgraphicsprogrammingMar 17, 2025•
edited
Loading

Choose a reason for hiding this comment

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

again, you're getting random floats based on workgroup index whichthankfully was always the same (rare case of two wrongs making a right)

Again if you wanted to stagger, you should use entire subgroup to load the values, then subgroup reduce them

just converting tofloat_t is not the correct way to decode, you should divide by themaxIncrement

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@nipunG314nipunG314nipunG314 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@devshgraphicsprogramming@nipunG314@keptsecret
[8]ページ先頭
©2009-2025 Movatter.jp