video.i_x_offset/i_y_offset breaks opengl

clearly unaligned copy artefact https://streams.videolan.org/streams/ogg/kiki_Theora_Vorbis_kate.ogg

Meanwhile x11/swscale complains of unaligned data. Maybe the crop is done too early.

added Component::Video: OpenGL label

(Regarding swscale, this is a limitation of the library. We cannot crop after scaling, as that would obviously cause bleeding of the cropped edges into the picture. The only solution would be to copy-crop explicitly before swscale.)

Breaks almost all theora files as Xiph, like everyone , requires crop on the left side of the pic.

gl_interop & chroma parameters related

Is this a regression from 3.0 or what?

regression. x/y offset now unhandled

added Severity::major label

changed milestone to %4.0

added Severity::critical label and removed Severity::major label

added Version::master git label

assigned to @rom1v

bisect/bad is 85f85230 from !1406 (merged):

commit 85f85230df103124658ae77df40991cfc84237b2
Author: Zhao Zhili <quinkblack@foxmail.com>
Date:   Thu Feb 17 14:37:01 2022 +0800

    opengl: importer: fix crop handling

Reverting it is sufficient to make it work on master. cc @quink

---

For information, here is the value of video_format_t for this sample:

{
    i_chroma = 808596553,
    i_width = 544,  
    i_height = 432,
    i_x_offset = 2,
    i_y_offset = 0,
    i_visible_width = 540,
    i_visible_height = 432, 
    i_bits_per_pixel = 12,
    i_sar_num = 64,
    i_sar_den = 45,
    i_frame_rate = 25,
    i_frame_rate_base = 1,
    i_rmask = 0,
    i_gmask = 0, i_bmask = 0,
    p_palette = 0x0, 
    orientation = ORIENT_TOP_LEFT,
    primaries = COLOR_PRIMARIES_BT601_525,
    transfer = TRANSFER_FUNC_SRGB,
    space = COLOR_SPACE_BT601, 
    color_range = COLOR_RANGE_LIMITED,
    chroma_location = CHROMA_LOCATION_UNDEF,
    multiview_mode = MULTIVIEW_2D,
    b_multiview_right_eye_first = false, 
    projection_mode = PROJECTION_MODE_RECTANGULAR,
    pose = {
        yaw = 0,
        pitch = 0,
        roll = 0,
        fov = 80
    },
    mastering = {
        primaries = {0, 0, 0, 0, 0, 0},
        white_point = {0, 0},
        max_luminance = 0,
        min_luminance = 0
    },
    lighting = {
        MaxCLL = 0,
        MaxFALL = 0
    },
    i_cubemap_padding = 0
}

From a MR aiming at providing top/left cropping.. that's confusing

Note that top/left cropping is already handled by the OpenGL "importer": https://code.videolan.org/videolan/vlc/-/blob/49ff728d48005aac6d87ecfb8f8114fe07dedd51/modules/video_output/opengl/importer.c#L371-402

Yes I have noticed that, but before the patch:

        float scale_w = glfmt->tex_widths[0] * interop->texs[0].w.den
                                             / interop->texs[0].w.num;
        float scale_h = glfmt->tex_heights[0] * interop->texs[0].h.den
                                              / interop->texs[0].h.num;

And

            glfmt->tex_widths[j]  = interop->fmt_out.i_visible_width  * interop->texs[j].w.num
                  / interop->texs[j].w.den;
            glfmt->tex_heights[j] = interop->fmt_out.i_visible_height * interop->texs[j].h.num
                  / interop->texs[j].h.den;

So

scale_w = interop->fmt_out.i_visible_width
scale_h = interop->fmt_out.i_visible_heigh

Then

 	float left   = (source->i_x_offset +                       0 ) / scale_w;
        float top    = (source->i_y_offset +                       0 ) / scale_h;
        float right  = (source->i_x_offset + source->i_visible_width ) / scale_w;
        float bottom = (source->i_y_offset + source->i_visible_height) / scale_h;

is equal to

 	float left   = (source->i_x_offset +                       0 ) / interop->fmt_out.i_visible_width;
        float top    = (source->i_y_offset +                       0 ) / interop->fmt_out.i_visible_heigh;
        float right  = (source->i_x_offset + source->i_visible_width ) / interop->fmt_out.i_visible_width;
        float bottom = (source->i_y_offset + source->i_visible_height) / interop->fmt_out.i_visible_heigh;

If I remember clear, it's not uncommon that source->i_visible_width equal interop->fmt_out.i_visible_width and so on. Then we got right and/or bottom > 1.0. It doesn't looks right.

I'm not saying that commit 85f85230 is bugfree. And it's known that x_offset/y_offset don't get handled consistently, for example, packetizer output top/left crop as right/bottom crop.

I agree with your reasoning. So your commit (bisect/bad) looks quite correct, I would only apply this small change (the full width is not only visible and left margin, but also right margin, this is unrelated to this bug):

diff --git a/modules/video_output/opengl/importer.c b/modules/video_output/opengl/importer.c
index 5ce0e0456f..aa7d803ab8 100644
--- a/modules/video_output/opengl/importer.c
+++ b/modules/video_output/opengl/importer.c
@@ -240,9 +240,9 @@ vlc_gl_importer_New(struct vlc_gl_interop *interop)
                   / interop->texs[j].w.den;
         GLsizei vh = interop->fmt_out.i_visible_height * interop->texs[j].h.num
                   / interop->texs[j].h.den;
-        GLsizei w = (interop->fmt_out.i_visible_width + interop->fmt_out.i_x_offset) * interop->texs[j].w.num
+        GLsizei w = interop->fmt_out.i_width  * interop->texs[j].w.num
                   / interop->texs[j].w.den;
-        GLsizei h = (interop->fmt_out.i_visible_height + interop->fmt_out.i_y_offset) *  interop->texs[j].h.num
+        GLsizei h = interop->fmt_out.i_height * interop->texs[j].h.num
                   / interop->texs[j].h.den;
         glfmt->visible_widths[j] = vw;
         glfmt->visible_heights[j] = vh;

---

After investigations, it seems that the problem observed with this sample is related to the software interop: the tex_widths and tex_heights passed as parameter are the full width/height (544×432), not the visible ones (540×432). But the GL_UNPACK_ROW_LENGTH value was computed assuming the parameters were the visible values, so it used a row length of 548 (544*544/540).

Removing these GL_UNPACK_ROW_LENGTH values "fixes" the problem:

diff --git a/modules/video_output/opengl/interop_sw.c b/modules/video_output/opengl/interop_sw.c
index 6669559b21..2c11dd43d9 100644
--- a/modules/video_output/opengl/interop_sw.c
+++ b/modules/video_output/opengl/interop_sw.c
@@ -201,12 +201,8 @@ tc_pbo_update(const struct vlc_gl_interop *interop, uint32_t textures[],
         priv->gl.ActiveTexture(GL_TEXTURE0 + i);
         priv->gl.BindTexture(interop->tex_target, textures[i]);
 
-        priv->gl.PixelStorei(GL_UNPACK_ROW_LENGTH, pic->p[i].i_pitch
-            * tex_width[i] / (pic->p[i].i_visible_pitch ? pic->p[i].i_visible_pitch : 1));
-
         priv->gl.TexSubImage2D(interop->tex_target, 0, 0, 0, tex_width[i], tex_height[i],
                                    interop->texs[i].format, interop->texs[i].type, NULL);
-        priv->gl.PixelStorei(GL_UNPACK_ROW_LENGTH, 0);
     }
 
     if (pic->i_planes == 1 && interop->tex_count == 2)
@@ -214,11 +210,8 @@ tc_pbo_update(const struct vlc_gl_interop *interop, uint32_t textures[],
         /* For YUV 4:2:2 formats, a single plane is uploaded into 2 textures */
         priv->gl.ActiveTexture(GL_TEXTURE1);
         priv->gl.BindTexture(interop->tex_target, textures[1]);
-        priv->gl.PixelStorei(GL_UNPACK_ROW_LENGTH, pic->p[0].i_pitch
-            * tex_width[1] / (pic->p[0].i_visible_pitch ? pic->p[0].i_visible_pitch : 1));
         priv->gl.TexSubImage2D(interop->tex_target, 0, 0, 0, tex_width[1], tex_height[1],
                                interop->texs[1].format, interop->texs[1].type, NULL);
-        priv->gl.PixelStorei(GL_UNPACK_ROW_LENGTH, 0);
     }
 
     /* turn off pbo */

But in the end there is a green bar on the right, so something is still wrong. Moreover, for software interop, we could copy only the relevant part of the input picture to the texture, so there's more work to do.

To remove the green bar, you need to ensure that there is no rounding to the right of the visible pitch border.

But in the end there is a green bar on the right, so something is still wrong. Moreover, for software interop, we could copy only the relevant part of the input picture to the texture, so there's more work to do.

We can upload only the relevant part, but then it would still have the same result with the other interop, and you'd need to re-upload (even if we already do that the way it's written) the picture to change the visibility setting, and uploading would change the size then too, which is harder to handle.

Here are more details about the problem.

When a SPU picture is created (by freetype.c, which calls in the end picture_Setup()), its i_pitch may be different from its i_visible_pitch, typically due to rounding. For example, if the format is {width=472, height=51, i_visible_width=472, i_visible_height=51}, then the plane i_visible_pitch = 1888 (472 × 4 bytes), but i_pitch = 1920 (480 × 4 bytes).

Here are raw values captured here:

(gdb) p *fmt
$1 = {i_chroma = 1094862674, i_width = 472, i_height = 51, i_x_offset = 0, i_y_offset = 0, i_visible_width = 472, i_visible_height = 51, 
  i_bits_per_pixel = 0, i_sar_num = 1, i_sar_den = 1, i_frame_rate = 0, i_frame_rate_base = 0, i_rmask = 0, i_gmask = 0, i_bmask = 0, p_palette = 0x0, 
  orientation = ORIENT_TOP_LEFT, primaries = COLOR_PRIMARIES_BT709, transfer = TRANSFER_FUNC_SRGB, space = COLOR_SPACE_BT709, 
  color_range = COLOR_RANGE_UNDEF, chroma_location = CHROMA_LOCATION_UNDEF, multiview_mode = MULTIVIEW_2D, b_multiview_right_eye_first = false, 
  projection_mode = PROJECTION_MODE_RECTANGULAR, pose = {yaw = 0, pitch = 0, roll = 0, fov = 80}, mastering = {primaries = {0, 0, 0, 0, 0, 0}, white_point = {
      0, 0}, max_luminance = 0, min_luminance = 0}, lighting = {MaxCLL = 0, MaxFALL = 0}, i_cubemap_padding = 0}
(gdb) p *p
$2 = {p_pixels = 0x0, i_lines = 64, i_pitch = 1920, i_pixel_pitch = 4, i_visible_lines = 51, i_visible_pitch = 1888}

In the OpenGL interop, this difference is compensated here (and in other places in the same file) to compute the actual row length (the number of pixels to skip to go to the next line vertically): the width (472) is multiplied by pitch / visible_pitch, so the GL_UNPACK_ROW_LENGTH is 480. Therefore, this works for this case (the SPU).

Note that 480 cannot be deduced from the format only (the plane_t i_pitch and i_visible_pitch are required): in the format, both i_width and i_visible_width are 472.

However, the difference between i_pitch and i_visible_pitch, computed respectively from the format i_width and i_visible_width can result from different causes:

• the rounding of the width by picture_Setup(), which impacts the actual pitch (e.g. rounding 472→480 makes each row take more bytes);
• the initial difference between i_width and i_visible_width, for example due to padding (e.g. if i_x_offset > 0 like in this issue), which DOES NOT impact the actual pitch (reducing the i_visible_width does not change the number of bytes used to store the picture).

Thus, in the interop, there is currently no way to get both cases correct (e.g. on master, the kiki_Theora_Vorbis_kate.ogg sample is broken, but the SPU are ok; this patch fixes the picture (it remains a green bar on the right to be investigated), but breaks the SPU).

For now, it's not clear to me how to fix the problem.

---

Btw, the name i_visible_pitch is confusing: the pitch (or stride) is the number of bytes to skip to point to the next line vertically. With this definition, a visible pitch is meaningless: there is always the same amount of bytes (or even pixels) to go to the next line, regardless of what is considered "visible". This is just a naming issue, since its meaning is documented: How many visible pixels are there? (implicitly, "in a row").

But it's still not clear how to use it properly. For example, if the format describes a picture with an offset:

             i_width
  |<------------------------------->|
  |       i_visible_width           |
  |    +-------------------+        |
  |    |                   |        |
  |    |                   |        |
  |<-->|                   |        |
   i_x_offset

plane_t does not expose any offset, so i_visible_pitch alone is quite meaningless.

I took a fresh look at this issue.

However, the difference between i_pitch and i_visible_pitch, computed respectively from the format i_width and i_visible_width can result from different causes:

• the rounding of the width by picture_Setup(), which impacts the actual pitch (e.g. rounding 472→480 makes each row take more bytes);
• the initial difference between i_width and i_visible_width, for example due to padding (e.g. if i_x_offset > 0 like in this issue), which DOES NOT impact the actual pitch (reducing the i_visible_width does not change the number of bytes used to store the picture).

This is the root cause: the initial difference between i_width and i_visible_width should not have impact on the plane_t content area.

plane_t does not expose any offset, so i_visible_pitch alone is quite meaningless.

Because i_visible_pitch does not define the visible pitch, but the plane content area before alignment.

I submitted a MR to fix the problem: !2879 (closed). See the description for more details.

mentioned in merge request !2551 (merged)

mentioned in issue #27382

mentioned in merge request !2879 (closed)

mentioned in merge request !2898 (merged)

closed with commit b9edc720

closed with merge request !2898 (merged)

mentioned in merge request !2908

mentioned in issue #19938

marked this issue as related to #19938

marked this issue as related to #27382