Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mark rendering improvements #13969

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+121 −103
Open

mark rendering improvements #13969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c5d52f
optimize images for marks: don't use empty space to align the mark
Nov 30, 2024
680b1ec
draw rounded to pixel
Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions src/waveform/renderers/allshader/digitsrenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ void allshader::DigitsRenderer::updateTexture(
QFont font;
QFontMetricsF metrics{font};
font.setFamily("Open Sans");
qreal totalTextWidth;
qreal maxTextHeight;
bool retry = false;
do {
Expand All @@ -107,14 +106,12 @@ void allshader::DigitsRenderer::updateTexture(

metrics = QFontMetricsF{font};

totalTextWidth = 0;
maxTextHeight = 0;

for (int i = 0; i < NUM_CHARS; i++) {
const QString text(indexToChar(i));
const auto rect = metrics.tightBoundingRect(text);
maxTextHeight = std::max(maxTextHeight, rect.height());
totalTextWidth += metrics.horizontalAdvance(text);
}
if (m_adjustedFontPointSize == 0.f && !retry && maxTextHeight > maxHeightWithoutSpace) {
// We need to adjust the font size to fit in the maxHeight.
Expand All @@ -131,12 +128,25 @@ void allshader::DigitsRenderer::updateTexture(

m_height = static_cast<float>(std::ceil(maxTextHeight) + space * 2 + 1);

// Space around the digits
totalTextWidth += (space * 2 + 1) * NUM_CHARS;
totalTextWidth = std::ceil(totalTextWidth);

const qreal y = maxTextHeight + space - 0.5;

qreal totalTextWidth{};
std::array<qreal, NUM_CHARS> xs;
for (int i = 0; i < NUM_CHARS; i++) {
const QString text(indexToChar(i));
xs[i] = totalTextWidth;
qreal w = std::round(metrics.horizontalAdvance(text) *
devicePixelRatio / devicePixelRatio) +
space + space + 1.0;
Comment on lines +138 to +140
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
qreal w = std::round(metrics.horizontalAdvance(text) *
devicePixelRatio / devicePixelRatio) +
space + space + 1.0;
qreal w = std::round(metrics.horizontalAdvance(text)) +
space + space + 1.0;

Or should the division be after the round?

totalTextWidth += w;
m_width[i] = static_cast<float>(w);
}
for (int i = 0; i < NUM_CHARS; i++) {
// position and width of character at index i in the texture, normalized
m_offset[i] = static_cast<float>(xs[i] / totalTextWidth);
}
m_offset[NUM_CHARS] = 1.f;

QImage image(std::lround(totalTextWidth * devicePixelRatio),
std::lround(m_height * devicePixelRatio),
QImage::Format_ARGB32_Premultiplied);
Expand All @@ -153,12 +163,10 @@ void allshader::DigitsRenderer::updateTexture(
painter.setBrush(QColor(0, 0, 0, OUTLINE_ALPHA));
painter.setPen(pen);
painter.setFont(font);
qreal x = 0;
QPainterPath path;
for (int i = 0; i < NUM_CHARS; i++) {
const QString text(indexToChar(i));
path.addText(QPointF(x + space + 0.5, y), font, text);
x += metrics.horizontalAdvance(text) + space + space + 1;
path.addText(QPointF(xs[i] + space + 0.5, y), font, text);
}
painter.drawPath(path);
}
Expand Down Expand Up @@ -187,19 +195,12 @@ void allshader::DigitsRenderer::updateTexture(
painter.setPen(Qt::white);
painter.setBrush(Qt::white);

qreal x = 0;
QPainterPath path;
for (int i = 0; i < NUM_CHARS; i++) {
const QString text(indexToChar(i));
path.addText(QPointF(x + space + 0.5, y), font, text);
// position and width of character at index i in the texture
m_offset[i] = static_cast<float>(x / totalTextWidth);
const auto xp = x;
x += metrics.horizontalAdvance(text) + space + space + 1;
m_width[i] = static_cast<float>(x - xp);
path.addText(QPointF(xs[i] + space + 0.5, y), font, text);
}
painter.drawPath(path);
m_offset[NUM_CHARS] = 1.f;
}

m_texture.setData(image);
Expand Down
99 changes: 49 additions & 50 deletions src/waveform/renderers/allshader/waveformrendermark.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,19 @@
// only to draw on a QImage. This is only done once when needed and the images are
// then used as textures to be drawn with a GLSL shader.

namespace {

struct RoundToPixel {
const float m_devicePixelRatio;
RoundToPixel(float devicePixelRatio)
: m_devicePixelRatio{devicePixelRatio} {
}
// round to nearest pixel, taking into account the devicePixelRatio
float operator()(float pos) const {
return std::round(pos * m_devicePixelRatio) / m_devicePixelRatio;
}
};
Comment on lines +25 to +34
Copy link
Member

Choose a reason for hiding this comment

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

NIT: you can make this a little more concise if you don't need to access devicePixelRatio afterwards.

Suggested change
struct RoundToPixel {
const float m_devicePixelRatio;
RoundToPixel(float devicePixelRatio)
: m_devicePixelRatio{devicePixelRatio} {
}
// round to nearest pixel, taking into account the devicePixelRatio
float operator()(float pos) const {
return std::round(pos * m_devicePixelRatio) / m_devicePixelRatio;
}
};
auto makeRoundToPixel(float devicePixelRatio) {
return [devicePixelRatio](float pos) {
return std::round(pos * devicePixelRatio) / devicePixelRatio;
};
}


class TextureGraphics : public WaveformMark::Graphics {
public:
TextureGraphics(const QImage& image) {
Expand All @@ -33,19 +46,9 @@
OpenGLTexture2D m_texture;
};

// Both allshader::WaveformRenderMark and the non-GL ::WaveformRenderMark derive
// from WaveformRenderMarkBase. The base-class takes care of updating the marks
// when needed and flagging them when their image needs to be updated (resizing,
// cue changes, position changes)
//
// While in the case of ::WaveformRenderMark those images can be updated immediately,
// in the case of allshader::WaveformRenderMark we need to do that when we have an
// openGL context, as we create new textures.
//
// The boolean argument for the WaveformRenderMarkBase constructor indicates
// that updateMarkImages should not be called immediately.
constexpr float kPlayPosWidth{11.f};
constexpr float kPlayPosOffset{-(kPlayPosWidth - 1.f) / 2.f};

namespace {
QString timeSecToString(double timeSec) {
int hundredths = std::lround(timeSec * 100.0);
int seconds = hundredths / 100;
Expand All @@ -58,6 +61,18 @@

} // namespace

// Both allshader::WaveformRenderMark and the non-GL ::WaveformRenderMark derive
// from WaveformRenderMarkBase. The base-class takes care of updating the marks
// when needed and flagging them when their image needs to be updated (resizing,
// cue changes, position changes)
//
// While in the case of ::WaveformRenderMark those images can be updated immediately,
// in the case of allshader::WaveformRenderMark we need to do that when we have an
// openGL context, as we create new textures.
//
// The boolean argument for the WaveformRenderMarkBase constructor indicates
// that updateMarkImages should not be called immediately.

allshader::WaveformRenderMark::WaveformRenderMark(
WaveformWidgetRenderer* waveformWidget,
::WaveformRendererAbstract::PositionSource type)
Expand Down Expand Up @@ -198,6 +213,8 @@
glEnable(GL_BLEND);
glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);

RoundToPixel roundToPixel{devicePixelRatio};

for (const auto& pMark : std::as_const(m_marks)) {
pMark->setBreadth(slipActive ? m_waveformRenderer->getBreadth() / 2
: m_waveformRenderer->getBreadth());
Expand Down Expand Up @@ -229,36 +246,27 @@
continue;
}

const float currentMarkPoint =
std::round(
static_cast<float>(
m_waveformRenderer
->transformSamplePositionInRendererWorld(
samplePosition, positionType)) *
devicePixelRatio) /
devicePixelRatio;
const float currentMarkPos = static_cast<float>(
m_waveformRenderer->transformSamplePositionInRendererWorld(
samplePosition, positionType));
if (pMark->isShowUntilNext() &&
samplePosition >= playPosition + 1.0 &&
samplePosition < nextMarkPosition) {
nextMarkPosition = samplePosition;
}
const double sampleEndPosition = pMark->getSampleEndPosition();

// Pixmaps are expected to have the mark stroke at the center,
// and preferably have an odd width in order to have the stroke
// exactly at the sample position.
const float markHalfWidth = pTexture->width() / devicePixelRatio / 2.f;
const float drawOffset = currentMarkPoint - markHalfWidth;
const float markWidth = pTexture->width() / devicePixelRatio;
const float drawOffset = currentMarkPos + pMark->getOffset();

bool visible = false;
// Check if the current point needs to be displayed.
if (drawOffset > -markHalfWidth &&
drawOffset < m_waveformRenderer->getLength() +
markHalfWidth) {
if (drawOffset > -markWidth &&
drawOffset < m_waveformRenderer->getLength()) {
drawTexture(matrix,
drawOffset,
roundToPixel(drawOffset),
!m_isSlipRenderer && slipActive
? m_waveformRenderer->getBreadth() / 2
? roundToPixel(m_waveformRenderer->getBreadth() / 2.f)
: 0,
pTexture);
visible = true;
Expand All @@ -267,19 +275,16 @@
// Check if the range needs to be displayed.
if (samplePosition != sampleEndPosition && sampleEndPosition != Cue::kNoPosition) {
DEBUG_ASSERT(samplePosition < sampleEndPosition);
const float currentMarkEndPoint = static_cast<
float>(
m_waveformRenderer
->transformSamplePositionInRendererWorld(
sampleEndPosition, positionType));

if (visible || currentMarkEndPoint > 0) {
const float currentMarkEndPos = static_cast<float>(
m_waveformRenderer->transformSamplePositionInRendererWorld(
sampleEndPosition, positionType));
if (visible || currentMarkEndPos > 0.f) {
QColor color = pMark->fillColor();
color.setAlphaF(0.4f);

drawMark(matrix,
QRectF(QPointF(currentMarkPoint, 0),
QPointF(currentMarkEndPoint,
QRectF(QPointF(roundToPixel(currentMarkPos), 0),
QPointF(roundToPixel(currentMarkEndPos),
m_waveformRenderer
->getBreadth())),
color);
Expand All @@ -295,24 +300,18 @@
}
m_waveformRenderer->setMarkPositions(marksOnScreen);

const float currentMarkPoint =
std::round(static_cast<float>(
m_waveformRenderer->getPlayMarkerPosition() *
m_waveformRenderer->getLength()) *
devicePixelRatio) /
devicePixelRatio;

const float playMarkerPos = m_waveformRenderer->getPlayMarkerPosition() *

Check warning on line 303 in src/waveform/renderers/allshader/waveformrendermark.cpp

View workflow job for this annotation

GitHub Actions / coverage 

conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
m_waveformRenderer->getLength();
if (m_playPosMarkTexture.isStorageAllocated()) {
const float markHalfWidth = m_playPosMarkTexture.width() / devicePixelRatio / 2.f;
const float drawOffset = currentMarkPoint - markHalfWidth;
const float drawOffset = roundToPixel(playMarkerPos + kPlayPosOffset);

drawTexture(matrix, drawOffset, 0.f, &m_playPosMarkTexture);
}

if (WaveformWidgetFactory::instance()->getUntilMarkShowBeats() ||
WaveformWidgetFactory::instance()->getUntilMarkShowTime()) {
updateUntilMark(playPosition, nextMarkPosition);
drawUntilMark(matrix, currentMarkPoint + 20);
drawUntilMark(matrix, roundToPixel(playMarkerPos + 20.f));
}
}

Expand Down Expand Up @@ -382,7 +381,7 @@

const float lineX = 5.5f;

imgwidth = 11.f;
imgwidth = kPlayPosWidth;
imgheight = height;

QImage image(static_cast<int>(imgwidth * devicePixelRatio),
Expand Down
56 changes: 38 additions & 18 deletions src/waveform/renderers/waveformmark.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
const WaveformSignalColors& signalColors,
int hotCue)
: m_linePosition{},
m_offset{},
m_breadth{},
m_level{},
m_iPriority(priority),
Expand Down Expand Up @@ -278,6 +279,15 @@
const Qt::Alignment alignH = align & Qt::AlignHorizontal_Mask;
const Qt::Alignment alignV = align & Qt::AlignVertical_Mask;
const bool alignHCenter{alignH == Qt::AlignHCenter};

// The image width is the label rect width + 1, so that the label rect
// left and right positions can be at an integer + 0.5. This is so that
// the label rect is drawn at an exact pixel positions.
//
// Likewise, the line position also has to fall on an integer + 0.5.
// When center aligning, the image width has to be odd, so that the
// center is an integer + 0.5. For the image width to be odd, to
// label rect width has to be even.
const qreal widthRounding{alignHCenter ? 2.f : 1.f};

m_labelRect = QRectF{0.f,
Expand All @@ -286,17 +296,9 @@
widthRounding,
std::ceil(capHeight + 2.f * margin)};

m_imageSize = QSizeF{alignHCenter ? m_labelRect.width() + 1.f
: 2.f * m_labelRect.width() + 1.f,
breadth};
m_imageSize = QSizeF{m_labelRect.width() + 1.f, breadth};

if (alignH == Qt::AlignHCenter) {
m_labelRect.moveLeft((m_imageSize.width() - m_labelRect.width()) / 2.f);
} else if (alignH == Qt::AlignRight) {
m_labelRect.moveRight(m_imageSize.width() - 0.5f);
} else {
m_labelRect.moveLeft(0.5f);
}
m_labelRect.moveLeft(0.5f);

const float increment = overlappingMarkerIncrement(
static_cast<float>(m_labelRect.height()), breadth);
Expand Down Expand Up @@ -373,22 +375,40 @@

painter.setWorldMatrixEnabled(false);

// Draw marker lines
const auto hcenter = markerGeometry.m_imageSize.width() / 2.f;
m_linePosition = static_cast<float>(hcenter);
const Qt::Alignment alignH = m_align & Qt::AlignHorizontal_Mask;
switch (alignH) {
case Qt::AlignHCenter:
m_linePosition = markerGeometry.m_imageSize.width() / 2.f;

Check failure on line 381 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 24.04 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Werror=float-conversion]

Check warning on line 381 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / coverage 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Wfloat-conversion]
m_offset = -(markerGeometry.m_imageSize.width() - 1.f) / 2.f;

Check failure on line 382 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 24.04 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Werror=float-conversion]

Check warning on line 382 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / coverage 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Wfloat-conversion]
break;
case Qt::AlignLeft:
m_linePosition = markerGeometry.m_imageSize.width() - 1.5f;

Check failure on line 385 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 24.04 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Werror=float-conversion]

Check warning on line 385 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / coverage 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Wfloat-conversion]
m_offset = -markerGeometry.m_imageSize.width() + 2.f;

Check failure on line 386 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 24.04 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Werror=float-conversion]

Check warning on line 386 in src/waveform/renderers/waveformmark.cpp

View workflow job for this annotation

GitHub Actions / coverage 

conversion from ‘qreal’ {aka ‘double’} to ‘float’ may change value [-Wfloat-conversion]
break;
case Qt::AlignRight:
default:
m_linePosition = 1.5f;
m_offset = -1.f;
break;
}

// Note: linePos has to be at integer + 0.5 to draw correctly
const float linePos = m_linePosition;
[[maybe_unused]] const float epsilon = 1e-6f;
DEBUG_ASSERT(std::abs(linePos - std::floor(linePos) - 0.5) < epsilon);

// Draw the center line
painter.setPen(fillColor());
painter.drawLine(QLineF(hcenter, 0.f, hcenter, markerGeometry.m_imageSize.height()));
painter.drawLine(QLineF(linePos, 0.f, linePos, markerGeometry.m_imageSize.height()));

painter.setPen(borderColor());
painter.drawLine(QLineF(hcenter - 1.f,
painter.drawLine(QLineF(linePos - 1.f,
0.f,
hcenter - 1.f,
linePos - 1.f,
markerGeometry.m_imageSize.height()));
painter.drawLine(QLineF(hcenter + 1.f,
painter.drawLine(QLineF(linePos + 1.f,
0.f,
hcenter + 1.f,
linePos + 1.f,
markerGeometry.m_imageSize.height()));

if (useIcon || label.length() != 0) {
Expand Down
5 changes: 5 additions & 0 deletions src/waveform/renderers/waveformmark.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class WaveformMark {
WaveformMark(const WaveformMark&) = delete;
WaveformMark& operator=(const WaveformMark&) = delete;

float getOffset() const {
return m_offset;
}

int getHotCue() const {
return m_iHotCue;
};
Expand Down Expand Up @@ -158,6 +162,7 @@ class WaveformMark {
QString m_iconPath;

float m_linePosition;
float m_offset;
float m_breadth;

// When there are overlapping marks, level is increased for each overlapping mark,
Expand Down
Loading
Loading