Multiple crops #32
Multiple crops #32
Conversation
Tiles may produce output on only a single branch when multiple branches are active. Track the completion using a new BranchComplete() helper, which traverses the pipeline till the output stage and returns a flag if the stage has completed all its output. In the split stage, do not inject tiles into a branch if the output is complete. Signed-off-by: Naushir Patuck <[email protected]>
For backward compatibility, we keep the existing BackEnd::SetCrop behaving the same and setting the same crop params for all outputs. Signed-off-by: Naushir Patuck <[email protected]>
… tile It is possible to have a configuration where a branch starts producing output after the first tile in a row/column. In such cases, the tile edge flag will not be set, and cause the resample to start filtering at the wrong pixel offset. Fix this by tracking when a branch has started producing output, and allowing neative input_start values to be used in PushStartUp() on all but the first tile in the row/column. Signed-off-by: Naushir Patuck <[email protected]>
naushir
force-pushed
the
multiple_crops
branch
2 times, most recently
from
560a14e
to
1904ed4
Compare
|
@davidplowman my torture tests are still running, likely only done by the weekend now. However I expect everything to pass now, so we ought to get ready to merge this soon. Given the magnitude of the changes, can you check through my work please? |
| { | ||
| started_ = false; | ||
| return !output_interval_.length; |
There was a problem hiding this comment.
I guess I feel slightly weird that BranchInactive() looks at the output_interval, but BranchComplete() doesn't - it slight offends my need for symmetry. Should BranchCompletel() look at output_interval_.End() or something? Or do I worry too much?
| @@ -46,7 +46,7 @@ class RescaleStage : public BasicStage | |||
|
|
|||
| private: | |||
| Config config_; | |||
| uint32_t round_up; | |||
There was a problem hiding this comment.
Ouch! That must have reacted badly to the negative offsets we now have...
|
Mostly this all looks pretty good to me. I guess I've seen most of it before. Though there are surprisingly few changes compared to the amount of head-scratching required to find them! I'm still wondering a little bit about the symmetry (or lack of) between |
Yes, perhaps we can replace this with |
Remove the started flag from the crop stage and rely on the output interval length instead. Add a BranchInactive() helper to the base stage class. When testing for the above, ensure we handle the case where the output interval end is > min tile size, but the length is not because of a possible negative offset. In BasicStage::CopyOut(), if a stage is inactive or complete, reset all intervals before copying out to the tile structures. Signed-off-by: Naushir Patuck <[email protected]>
The round_up const was unsigned whereas the offsets might be signed negative, causing an overflow. Fix this by changing the round_up const to a signed int. Signed-off-by: Naushir Patuck <[email protected]>
Also mark functions as override to keep clang happy. Signed-off-by: Naushir Patuck <[email protected]>
Remove OutputStage::Done() and explicitly test the output interval size from the top level pipeline. We also set the branch complete flag with a new helper OutputStage::SetBranchComplete() to be explicit. Rename OutputStage::BranchComplete() to OutputStage::GetBranchComplete() for consistency. Signed-off-by: Naushir Patuck <[email protected]>
|
@davidplowman 3a61f12 makes things neater with the branch complete flag. I think this is now good to go. |
No description provided.