plymouth-lite: fix a bug causing issues with framebuffer modes that are not /16

[samnazarko]

ply-lite by default does not handle some resolutions, like 1366x768.

try hdmi_mode=81 to replicate. This patch hopefully resolves that issue.

[stefansaraev]

I dont know what is the bug you are talking about, just tested a generic x86_64 build on intel, laptop screen with max resolution 1366x768 and it (ninja edit: this pr) introduces a bug for me

http://i.imgur.com/88VGuOs.jpg

[fritsch]

Can you provide dmesg | pastebinit from this boot?

intel kms overwritten?

2015-06-30 19:33 GMT+02:00 Stefan Saraev notifications@github.com:

I dont know what is the bug you are talking about, just tested a generic
x86_64 build on intel, laptop screen with max resolution 1366x768 and it
introduces a bug for me

http://i.imgur.com/88VGuOs.jpg

—
Reply to this email directly or view it on GitHub
#4221 (comment)
.

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

[MilhouseVH]

It's cropped up on the Kodi forum from time to time, eg. here in relation to OSMC, and here, in relation to OpenELEC on RPi. Not sure if it affects other architectures.

Edit: Just seen @stefansaraev's reply, as he has it on x86.

[popcornmix]

The bug is that plymouth-lite assumes that you add width*bytes_per_pixel to move down one pixel.
This is not necessarily the case. Image buffers usually have a separate parameter (pitch or stride) that describes how to move down a pixel. It is common for the pitch to be the width aligned up to 16 times the bytes_per_pixel (as is the case on the Pi's framebuffer driver).

I believe fbset -s reports the pitch of the framebuffer as "LineLength".

[stefansaraev]

@fritsch

Can you provide dmesg | pastebinit from this boot?

nothing useful here.

@MilhouseVH it is this PR that intruduces the same bug to x86. without this PR - all good. no bug (rebuilt and retested 3 times) :)

EDIT: so this fix hides some other bug (endianess? memory alignment? something very rpi-specific? did someone test without arm-mem:init btw? ninja edit: arm-mem is shot in the dark, no fingers pointed..)

[MilhouseVH]

@MilhouseVH it is this PR that intruduces the same bug to x86. without this PR - all good. no bug (rebuilt and retested 3 times) :)

Aha... :)

There does seem to be an issue somewhere, though.

[popcornmix]

The bug is that a kernel framebuffer driver exports a value called line_length through the FBIOGET_FSCREENINFO ioctl that should be used to determine the pitch of the framebuffer. See:
https://github.com/nemomobile-packages/fbset/blob/master/fbset/fbset.c#L781

plymouth-lite doesn't query that value, but guesses it's probably width * bytes_per_pixel. Which it sometimes is (e.g. on @stefansaraev's setup), but that's not a valid assumption to make.

This PR is not the correct solution (although it does the correct thing on the Pi).
The correct solution is to read the pitch from the FBIOGET_FSCREENINFO ioctl.

[stefansaraev]

@samnazarko here is my suggestion, move it to project specific patches - projects/RPi*/patches/ so x86 people (including me) dont have to care about rpi specifics, potentialy breaking other platforms

[samnazarko]

@fritsch

• I was not aware of an upstream repository that is still being maintained. We maintain our own source so I just forked this: https://github.com/nemomobile/plymouth-lite. OE seems to have downstream patches too

[MilhouseVH]

@stefansaraev Which is a great solution, until some other non-x86/non-RPi platform experiences a similar problem. It sounds like adding the ioctl query would ensure it works for every platform, regardless. Perhaps if Sam can rework the patch to include the ioctl query then it shouldn't need to be project specific.

[fritsch]

Popcornmix is fully right.

A real fix should read LineLength and see if 4 * LineLength == width, if not align to 16 bit.

@MilhouseVH Can you give me the relevant output from the Pi? fbset -i -a?

[samnazarko]

ply-lite already populates a fb_fix_screeninfo struct in

static bool ply_frame_buffer_query_device (ply_frame_buffer_t *buffer)

The stride is also calculated:

buffer->row_stride = fixed_screen_info.line_length / buffer->bytes_per_pixel;

[MilhouseVH]

With Kodi up and running on a 1920x1080 display I'm getting:

OpenELEC (unofficial) Version: devel-20150629210343-#0629-g398d3e2
OpenELEC git: 398d3e2339a85dce1fdca331febe5bff5dccec22
rpi22:~ # fbset -i -a
rpi22:~ # fbset -s

mode "1x1-0"
        # D: 0.000 MHz, H: 0.000 kHz, V: 0.000 Hz
        geometry 1 1 1 1 32
        timings 0 0 0 0 0 0 0
        accel false
        rgba 8/16,8/8,8/0,8/24
endmode

however the framebuffer may be configured differently when the OE splash is being shown.

[samnazarko]

mode "1x1-0" likely means the framebuffer got resized and (probably unbound) to save memory as Kodi launched.

[popcornmix]

I suspect setting width = fixed_screen_info.line_length / buffer->bytes_per_pixel for the new_image may be right (although I can't see the whole source for ply-image.c here - I'll have to dig that out).

[fritsch]

That's the problem in deed, the image creation does not care about the fixed_screen_info at all .. which is the root cause of the problem.

[popcornmix]

@MilhouseVH I think that is busybox's fbset which doesn't support all the options of debian's version.

[MilhouseVH]

@samnazarko: Yeah it doesn't look useful, I suspect I'd need to hack something into init to get useful info... @fritsch if you need this, let me know, although wouldn't the output be resolution specific (I don't have the problem that has been reported, and can't reproduce with hdmi_mode=81)

@popcornmix: OK so probably not worth hacking something into init either, in that case.

[stefansaraev]

@MilhouseVH here some non-x86 non-rpi

OpenELEC (unofficial) Version: devel-20150630190254-r21068-g1d9fd68
OpenELEC git: 1d9fd68d5ac0310af3f4164abccc1724c03ef400
Wetek:~ # fbset -i -a
Wetek:~ # fbset -s

mode "1280x720-0"
        # D: 0.000 MHz, H: 0.000 kHz, V: 0.000 Hz
        geometry 1280 720 1920 2160 32
        timings 0 0 0 0 0 0 0
        accel false
        rgba 8/16,8/8,8/0,8/24
endmode

[fritsch]

I think calling: ply_frame_buffer_query_device before creating the image and then using stride instead of width it should work?

[samnazarko]

in which case, we can just use buffer->row_stride.

[popcornmix]

For interest DMT mode 81 on Pi in 16bpp mode has

pi@raspberrypi:~ $ fbset  -i

mode "1366x768"
    geometry 1366 768 1366 768 16
    timings 0 0 0 0 0 0 0
    rgba 5/11,6/5,5/0,0/16
endmode

Frame buffer device information:
    Name        : BCM2708 FB
    Address     : 0x3d9f6000
    Size        : 2113536
    Type        : PACKED PIXELS
    Visual      : TRUECOLOR
    XPanStep    : 1
    YPanStep    : 1
    YWrapStep   : 0
    LineLength  : 2752
    Accelerator : No

Note that LineLength is 2752 which is not 1366 * 2 (it is 1376 * 2, i.e. width rounded up by 16).
The LineLength needs to be used as the pitch.

[samnazarko]

updated patch which multplies height against (fixed_screen_info.line_length / buffer->bytes_per_pixel;). Is that sufficient?

[fritsch]

Looks fine, can you make the parameter unsigned int please, then we save us a warning - as the original stride is also unsigned int?

if @popcornmix is fine with that change I am also fine with it.

[MilhouseVH]

Assuming all is well with the patch it would be good if it could also go upstream - I had a chat on #nemomobile and they'd be willing to take a look. Thanks Sam.

[fritsch]

Yes, thanks very much - @MilhouseVH get them here. Cause the patch needs to be applied in three other functions, too.

[fritsch]

And before I forget: They should also have a look at our "custom" patches. Upstream is the place for those, also.

[samnazarko]

@fritsch we have custom patches too. Is https://github.com/nemomobile/plymouth-lite the upstream? There is not much activity there.

[MilhouseVH]

@fritsch: I guess we should be sending them the pull requests in that case.

@samnazarko: It's not entirely clear from the OpenELEC package.mk, as it references meego.com but I suspect this is the correct upstream.

[fritsch]

I think calling: ply_frame_buffer_query_device before creating the image and then using stride instead of width it should work?

@samnazarko ^^ this is missing in your code - I think that will now segfault, right?

[samnazarko]

I don't think so, because buffer->height, buffer->width was already being passed to ply_image_resize in main(), and looking at ply_frame_buffer_query_device it shows that the buffer struct is completely populated in that function, so it must have been configured earlier.

[popcornmix]

Patch looks reasonable to me. Using stride rather than width*4 is correct.
Assuming it works okay then fine by me.

[MilhouseVH]

Sorry about the really poor camerawork but this is what I get with an RPi2 and the current patch - I'm using a 1920x1080 monitor:

[MilhouseVH]

For what it's worth, while on that screen, fbset -s returns:

mode "1920x1080-0"
        # D: 0.000 MHz, H: 0.000 kHz, V: 0.000 Hz
        geometry 1920 1080 1920 1080 16
        timings 0 0 0 0 0 0 0
        accel false
        rgba 5/11,6/5,5/0,0/16
endmode

fbset -i -a doesn't output any results.

[fritsch]

ply-frame-buffer.c: buffer->row_stride = fixed_screen_info.line_length / buffer->bytes_per_pixel

so if one wants the size in bytes again -> multiply with
buffer->bytes_per_pixel ?

2015-06-30 22:01 GMT+02:00 MilhouseVH notifications@github.com:

For what it's worth, while on that screen, fbset -s returns:

mode "1920x1080-0"
# D: 0.000 MHz, H: 0.000 kHz, V: 0.000 Hz
geometry 1920 1080 1920 1080 16
timings 0 0 0 0 0 0 0
accel false
rgba 5/11,6/5,5/0,0/16
endmode

fbset -i -a doesn't output any results.

—
Reply to this email directly or view it on GitHub
#4221 (comment)
.

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

[fritsch]

Oki - thx @MilhouseVH for veryfiing.

The problem here is, the stride is not really the stride, as it is a scalar value more something like the "aligned width". So you need to multiply it with 4 in order to get the correct aligend width in bytes again. 4 cause of the data type RGBA that is created here.

[samnazarko]

3rd time is the charm?

[MilhouseVH]

I tested a couple of variations of the row_stride value, at @fritsch's request:

image = ply_image_resize(image, buffer->area.width, buffer->area.height, buffer->row_stride * buffer->bytes_per_pixel);

isn't working, and produces the following image:

image = ply_image_resize(image, buffer->area.width, buffer->area.height, buffer->row_stride * 4);

is displaying the splash image correctly (which can be seen here for anyone unfamiliar with the OpenELEC splash image)

[fritsch]

@samnazarko sadly not. Keep the stride parameter and just multiply with 4 later on.

We have to distinguish between the fb configuration which most likely has 2 bytes only (16 bit) and the rgba we are creating here with 32 bits (4 Bytes).

This thingy is a real hack and it's usage is not fully clear :-) so, remove the bytes and just multiply with 4.

Then we test it in nightly builds.

[samnazarko]

done

[fritsch]

@samnazarko Thanks much - now let's get it tested.

[MilhouseVH]

Thanks Sam, I'll include this in tonight's test builds which should get us a bit of coverage, at least on RPi/RPi2.

[popcornmix]

I've spent a while trying to reproduce this. I can't reproduce the issue with a 32bpp framebuffer.
With (default) 16bpp framebuffer and:

hdmi_group=2
hdmi_mode=81

I get the familiar slopey image effect:
https://dl.dropboxusercontent.com/u/3669512/temp/snapshot.png

The bug is actually in https://github.com/OpenELEC/OpenELEC.tv/blob/master/packages/tools/plymouth-lite/patches/plymouth-lite-0.6.0-21-16bpp.patch

not in upstream plymouth-lite.

It is effectively the bug we've been talking about (using width rather than pitch). I'd suggest this to fix it:
https://gist.github.com/popcornmix/c0c8dbdc40870edac83a

(edit: updated to fix both the 16bpp and 32bpp versions)

This is an obvious bug. We are correctly using buffer->row_stride to move down to y1, but from then on we are just using the width.

(BTW: I've also removed the debug printf which I suspect isn't desired).

[MilhouseVH]

Thanks @popcornmix.

I've been able to reproduce the sloped image effect on a Pi2 with:

hdmi_group=2
hdmi_mode=81

I'm still able to reproduce the sloped image when using afd8b04 (this 4221 patch) and the existing plymouth-lite-0.6.0-21-16bpp patch.

By using the revised plymouth-lite-0.6.0-21-16bpp patch (c0c8dbdc), with or without afd8b04, the sloped image is no longer reproducible, so yes, it looks like we don't need afd8b04, and OpenELEC just need to revise the plymouth-lite-0.6.0-21-16bpp patch (I've submitted #4223)

It appears that OSMC already includes most of the defective plymouth-lite-0.6.0-21-16bpp patch in their tree, which would explain why both OSMC and OpenELEC were affected.

[fritsch]

@popcornmix thanks much for the fix
@MilhouseVH and @samnazarko thanks much for pointing out the issue and testing possible fixes. I will close now and +1 @MilhouseVH's newly opened PR.