Skip to content

Commit

Permalink
Fix rendering bugs caused by intersecting rects with different mipMap…
Browse files Browse the repository at this point in the history 
…Levels. (#918)

* Minor cleanup to help make it easier to verify correctness of fix.

- Updated rod computation so that rod could be declared as const.
  This is to make it easier to prove that this variable does not
  change beyond this point.

- Moved initial roi computation down to the image bounds computation
  code since that is where it is initially used and gets updated.

- Introduced isSupportedRenderScale() helper to remove duplicate code
  related to RenderScale support.

* Fix rendering bugs caused by intersecting rects with different mipMapLevels.

This change fixes 2 bugs where RectI objects containing information for
different mipMapLevels were being intersected with eachother. This was
causing visible compositing errors in the viewer when viewing the image
zoomed out and partially clipped by the edge of the viewer. This fix
just makes sure the RectIs are converted to the proper mipMapLevel before
being intersected.
  • Loading branch information
@acolwell
acolwell committed Sep 11, 2023
1 parent cc07dc6 commit 77b2b2f
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 68 deletions.
8 changes: 3 additions & 5 deletions Engine/EffectInstance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ EffectInstance::retrieveGetImageDataUponFailure(const double time,
getRegionsOfInterest(time, scale, optionalBounds, optionalBounds, ViewIdx(0), inputRois_p);
}

assert( !( (supportsRenderScaleMaybe() == eSupportsNo) && !(scale.x == 1. && scale.y == 1.) ) );
assert(isSupportedRenderScale(supportsRenderScaleMaybe(), scale));
const RectI pixelRod = rod.toPixelEnclosing(scale, getAspectRatio(-1));
try {
int identityInputNb;
Expand Down Expand Up @@ -1192,7 +1192,7 @@ EffectInstance::getRegionOfDefinition(U64 hash,
bool firstInput = true;
RenderScale renderMappedScale = scale;

assert( !( (supportsRenderScaleMaybe() == eSupportsNo) && !(scale.x == 1. && scale.y == 1.) ) );
assert(isSupportedRenderScale(supportsRenderScaleMaybe(), scale));

for (int i = 0; i < getNInputs(); ++i) {
if ( isInputMask(i) ) {
Expand Down Expand Up @@ -2402,7 +2402,7 @@ EffectInstance::Implementation::renderHandler(const EffectTLSDataPtr& tls,
actionArgs.byPassCache = byPassCache;
actionArgs.processChannels = processChannels;
actionArgs.mappedScale.x = actionArgs.mappedScale.y = Image::getScaleFromMipMapLevel( firstPlane.renderMappedImage->getMipMapLevel() );
assert( !( (_publicInterface->supportsRenderScaleMaybe() == eSupportsNo) && !(actionArgs.mappedScale.x == 1. && actionArgs.mappedScale.y == 1.) ) );
assert(isSupportedRenderScale(_publicInterface->supportsRenderScaleMaybe(), actionArgs.mappedScale));
actionArgs.originalScale.x = Image::getScaleFromMipMapLevel(mipMapLevel);
actionArgs.originalScale.y = actionArgs.originalScale.x;
actionArgs.draftMode = frameArgs->draftMode;
Expand Down Expand Up @@ -3751,8 +3751,6 @@ EffectInstance::isIdentity_public(bool useIdentityCache, // only set to true whe
ViewIdx* inputView,
int* inputNb)
{
//assert( !( (supportsRenderScaleMaybe() == eSupportsNo) && !(scale.x == 1. && scale.y == 1.) ) );

if (useIdentityCache) {
double timeF = 0.;
bool foundInCache = _imp->actionsCache->getIdentityResult(hash, time, view, inputNb, inputView, &timeF);
Expand Down
8 changes: 8 additions & 0 deletions Engine/EffectInstance.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,14 @@ GCC_DIAG_SUGGEST_OVERRIDE_ON
**/
virtual void getFrameRange(double *first, double *last);

/**
* @brief Returns true if the value of renderScale is supported for the given the value of supportsRS.
**/
static bool isSupportedRenderScale(SupportsEnum supportsRS, const RenderScale renderScale)
{
return (supportsRS != eSupportsNo) || (renderScale.x == 1. && renderScale.y == 1.);
}

public:

StatusEnum getRegionOfDefinition_public(U64 hash,
Expand Down
93 changes: 45 additions & 48 deletions Engine/EffectInstanceRenderRoI.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
renderMappedMipMapLevel = args.mipMapLevel;
}
RenderScale renderMappedScale( Image::getScaleFromMipMapLevel(renderMappedMipMapLevel) );
assert( !( (supportsRS == eSupportsNo) && !(renderMappedScale.x == 1. && renderMappedScale.y == 1.) ) );
assert(isSupportedRenderScale(supportsRS, renderMappedScale));


const FrameViewRequest* requestPassData = 0;
Expand All @@ -397,26 +397,27 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////// Get the RoD ///////////////////////////////////////////////////////////////
RectD rod; //!< rod is in canonical coordinates

RectD rodNc; //!< rod is in canonical coordinates
bool isProjectFormat = false;
{
///if the rod is already passed as parameter, just use it and don't call getRegionOfDefinition
if ( !args.preComputedRoD.isNull() ) {
rod = args.preComputedRoD;
rodNc = args.preComputedRoD;
} else {
///Check if the pre-pass already has the RoD
if (requestPassData) {
rod = requestPassData->globalData.rod;
rodNc = requestPassData->globalData.rod;
isProjectFormat = requestPassData->globalData.isProjectFormat;
} else {
assert( !( (supportsRS == eSupportsNo) && !(renderMappedScale.x == 1. && renderMappedScale.y == 1.) ) );
StatusEnum stat = getRegionOfDefinition_public(nodeHash, args.time, renderMappedScale, args.view, &rod, &isProjectFormat);
assert(isSupportedRenderScale(supportsRS, renderMappedScale));
StatusEnum stat = getRegionOfDefinition_public(nodeHash, args.time, renderMappedScale, args.view, &rodNc, &isProjectFormat);

///The rod might be NULL for a roto that has no beziers and no input
if (stat == eStatusFailed) {
///if getRoD fails, this might be because the RoD is null after all (e.g: an empty Roto node), we don't want the render to fail
return eRenderRoIRetCodeOk;
} else if ( rod.isNull() ) {
} else if ( rodNc.isNull() ) {
//Nothing to render
return eRenderRoIRetCodeOk;
}
Expand All @@ -432,18 +433,9 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
}
}
}
const RectD rod = rodNc;
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////// End get RoD ///////////////////////////////////////////////////////////////
RectI roi;
{
if (renderFullScaleThenDownscale) {
//We cache 'image', hence the RoI should be expressed in its coordinates
//renderRoIInternal should check the bitmap of 'image' and not downscaledImage!
roi = args.roi.toNewMipMapLevel(args.mipMapLevel, 0, par, rod);
} else {
roi = args.roi;
}
}

///Determine needed planes
ComponentsNeededMapPtr neededComps = std::make_shared<ComponentsNeededMap>();
Expand Down Expand Up @@ -553,7 +545,7 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
double inputTimeIdentity = 0.;
int inputNbIdentity;
ViewIdx inputIdentityView(args.view);
assert( !( (supportsRS == eSupportsNo) && !(renderMappedScale.x == 1. && renderMappedScale.y == 1.) ) );
assert(isSupportedRenderScale(supportsRS, renderMappedScale));
bool identity;
const RectI pixelRod = rod.toPixelEnclosing(args.mipMapLevel, par);
ViewInvarianceLevel viewInvariance = isViewInvariant();
Expand Down Expand Up @@ -736,39 +728,44 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////// Compute RoI depending on render scale ///////////////////////////////////////////////////


RectI downscaledImageBoundsNc = rod.toPixelEnclosing(args.mipMapLevel, par);
RectI upscaledImageBoundsNc = rod.toPixelEnclosing(0, par);
{
///Make sure the RoI falls within the image bounds
///Intersection will be in pixel coordinates
if (frameArgs->tilesSupported) {
if (renderFullScaleThenDownscale) {
if ( !roi.clipIfOverlaps(upscaledImageBoundsNc) ) {
return eRenderRoIRetCodeOk;
}
assert(roi.x1 >= upscaledImageBoundsNc.x1 && roi.y1 >= upscaledImageBoundsNc.y1 &&
roi.x2 <= upscaledImageBoundsNc.x2 && roi.y2 <= upscaledImageBoundsNc.y2);
} else {
if ( !roi.clipIfOverlaps(downscaledImageBoundsNc) ) {
return eRenderRoIRetCodeOk;
}
assert(roi.x1 >= downscaledImageBoundsNc.x1 && roi.y1 >= downscaledImageBoundsNc.y1 &&
roi.x2 <= downscaledImageBoundsNc.x2 && roi.y2 <= downscaledImageBoundsNc.y2);
}
#ifndef NATRON_ALWAYS_ALLOCATE_FULL_IMAGE_BOUNDS
///just allocate the roi
upscaledImageBoundsNc.clipIfOverlaps(roi);
downscaledImageBoundsNc.clipIfOverlaps(args.roi);
#endif
RectI roi;

if (renderFullScaleThenDownscale) {
//We cache 'image', hence the RoI should be expressed in its coordinates
//renderRoIInternal should check the bitmap of 'image' and not downscaledImage!
roi = args.roi.toNewMipMapLevel(args.mipMapLevel, 0, par, rod);

if (frameArgs->tilesSupported && !roi.clipIfOverlaps(upscaledImageBoundsNc)) {
return eRenderRoIRetCodeOk;
}
assert( upscaledImageBoundsNc.contains(roi));
} else {
roi = args.roi;

if (frameArgs->tilesSupported && !roi.clipIfOverlaps(downscaledImageBoundsNc)) {
return eRenderRoIRetCodeOk;
}
assert(downscaledImageBoundsNc.contains(roi));
}

/*
* Keep in memory what the user as requested, and change the roi to the full bounds if the effect doesn't support tiles
* Keep in memory what the user has requested, and change the roi to the full bounds if the effect doesn't support tiles
*/
const RectI originalRoI = roi;
if (!frameArgs->tilesSupported) {
// Store the mipMapLevel for originalRoI here because renderFullScaleThenDownscale may change later and we'll lose
// the ability to recover this information when we potentially need it for a downscale operation later.
const unsigned int originalRoIMipMapLevel = renderFullScaleThenDownscale ? 0 : args.mipMapLevel;

if (frameArgs->tilesSupported) {
#ifndef NATRON_ALWAYS_ALLOCATE_FULL_IMAGE_BOUNDS
///just allocate the roi
const RectI upscaledRoI = renderFullScaleThenDownscale ? roi : roi.toNewMipMapLevel(args.mipMapLevel, 0, par, rod);
upscaledImageBoundsNc.clipIfOverlaps(upscaledRoI);
downscaledImageBoundsNc.clipIfOverlaps(args.roi);
#endif
} else {
roi = renderFullScaleThenDownscale ? upscaledImageBoundsNc : downscaledImageBoundsNc;
}

Expand Down Expand Up @@ -1199,10 +1196,9 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
if (!input) {
continue;
}
bool isProjectFormat;
ParallelRenderArgsPtr inputFrameArgs = input->getParallelRenderArgsTLS();
U64 inputHash = (inputFrameArgs) ? inputFrameArgs->nodeHash : input->getHash();
StatusEnum stat = input->getRegionOfDefinition_public(inputHash, args.time, args.scale, args.view, &inputRod, &isProjectFormat);
StatusEnum stat = input->getRegionOfDefinition_public(inputHash, args.time, args.scale, args.view, &inputRod, nullptr);
if ( (stat != eStatusOK) && !inputRod.isNull() ) {
break;
}
Expand Down Expand Up @@ -1757,7 +1753,8 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
assert(comp);
///The image might need to be converted to fit the original requested format
if (comp) {
it->second.downscaleImage = convertPlanesFormatsIfNeeded(getApp(), it->second.downscaleImage, originalRoI, *comp, args.bitdepth, useAlpha0ForRGBToRGBAConversion, planesToRender->outputPremult, -1);
const RectI downscaledOriginalRoI = originalRoI.toNewMipMapLevel(originalRoIMipMapLevel, args.mipMapLevel, par, rod);
it->second.downscaleImage = convertPlanesFormatsIfNeeded(getApp(), it->second.downscaleImage, downscaledOriginalRoI, *comp, args.bitdepth, useAlpha0ForRGBToRGBAConversion, planesToRender->outputPremult, -1);
assert(it->second.downscaleImage->getComponents() == *comp && it->second.downscaleImage->getBitDepth() == args.bitdepth);

StorageModeEnum imageStorage = it->second.downscaleImage->getStorageMode();
Expand Down Expand Up @@ -1909,7 +1906,7 @@ EffectInstance::renderRoIInternal(EffectInstance* self,


if (callBegin) {
assert( !( (self->supportsRenderScaleMaybe() == eSupportsNo) && !(renderMappedScale.x == 1. && renderMappedScale.y == 1.) ) );
assert( self->isSupportedRenderScale(self->supportsRenderScaleMaybe(), renderMappedScale) );
if (self->beginSequenceRender_public(time, time, 1, !appPTR->isBackground(), renderMappedScale, isSequentialRender,
isRenderMadeInResponseToUserInteraction, frameArgs->draftMode, view, planesToRender->useOpenGL, planesToRender->glContextData) == eStatusFailed) {
renderStatus = eRenderingFunctorRetFailed;
Expand Down Expand Up @@ -1997,7 +1994,7 @@ EffectInstance::renderRoIInternal(EffectInstance* self,

///never call endsequence render here if the render is sequential
if (callBegin) {
assert( !( (self->supportsRenderScaleMaybe() == eSupportsNo) && !(renderMappedScale.x == 1. && renderMappedScale.y == 1.) ) );
assert( self->isSupportedRenderScale(self->supportsRenderScaleMaybe(), renderMappedScale) );
if (self->endSequenceRender_public(time, time, time, false, renderMappedScale,
isSequentialRender,
isRenderMadeInResponseToUserInteraction,
Expand Down
21 changes: 6 additions & 15 deletions Engine/OfxEffectInstance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1598,7 +1598,7 @@ OfxEffectInstance::getRegionsOfInterest(double time,
Q_UNUSED(outputRoD);
assert(renderWindow.x2 >= renderWindow.x1 && renderWindow.y2 >= renderWindow.y1);

assert((supportsRenderScaleMaybe() != eSupportsNo) || (scale.x == 1. && scale.y == 1.));
assert(isSupportedRenderScale(supportsRenderScaleMaybe(), scale));

OfxStatus stat;

Expand Down Expand Up @@ -1828,11 +1828,10 @@ OfxEffectInstance::isIdentity(double time,
// isIdentity may be the first action with renderscale called on any effect.
// it may have to check for render scale support.
SupportsEnum supportsRS = supportsRenderScaleMaybe();
bool scaleIsOne = (scale.x == 1. && scale.y == 1.);
if ( (supportsRS == eSupportsNo) && !scaleIsOne ) {
qDebug() << "isIdentity called with render scale != 1, but effect does not support render scale!";
if (!isSupportedRenderScale(supportsRS, scale) ) {
qDebug() << "isIdentity called with an unsupported RenderScale!";
assert(false);
throw std::logic_error("isIdentity called with render scale != 1, but effect does not support render scale!");
throw std::logic_error("isIdentity called with an unsupported RenderScale");
}

unsigned int mipMapLevel = Image::getLevelFromScale(scale.x);
Expand Down Expand Up @@ -1944,11 +1943,7 @@ OfxEffectInstance::beginSequenceRender(double first,
bool isOpenGLRender,
const EffectInstance::OpenGLContextEffectDataPtr& glContextData)
{
{
bool scaleIsOne = (scale.x == 1. && scale.y == 1.);
assert( !( (supportsRenderScaleMaybe() == eSupportsNo) && !scaleIsOne ) );
Q_UNUSED(scaleIsOne);
}
assert(isSupportedRenderScale(supportsRenderScaleMaybe(), scale));

OfxStatus stat;
unsigned int mipMapLevel = Image::getLevelFromScale(scale.x);
Expand Down Expand Up @@ -1990,11 +1985,7 @@ OfxEffectInstance::endSequenceRender(double first,
bool isOpenGLRender,
const EffectInstance::OpenGLContextEffectDataPtr& glContextData)
{
{
bool scaleIsOne = (scale.x == 1. && scale.y == 1.);
assert( !( (supportsRenderScaleMaybe() == eSupportsNo) && !scaleIsOne ) );
Q_UNUSED(scaleIsOne);
}
assert(isSupportedRenderScale(supportsRenderScaleMaybe(), scale));

OfxStatus stat;
unsigned int mipMapLevel = Image::getLevelFromScale(scale.x);
Expand Down

0 comments on commit 77b2b2f

Please sign in to comment.