[NTOS:INBV] Implement rotation bar for boot screen #194

binarymaster · 2017-12-14T12:58:25Z

Purpose

Implement a rotation bar for bootup screen! 😃

Inspired by JIRA issue: CORE-14101

Fixes issue: CORE-10327

Proposed changes

It adds implementation of bootup rotation bar, like in Windows XP.

Also implemented a rotation line animation which would be showed along with classic progress bar.

TODO

Any help is appreciated.

Figure out reason of BSOD 0x0000007E and fix it
Create system thread for rotation drawing code
Figure out reason of another BSOD 0x0000007E and fix it
Animation starts with a small delay, improve it
Implement a new animation to save original progress bar
Reduce patch diff for better compatibility

HBelusca · 2017-12-14T14:29:44Z

A fourth TODO would be to have this appearance set via a registry key or any other sort of setting :)

HBelusca · 2017-12-14T14:31:05Z

ntoskrnl/inbv/inbv.c

@@ -768,6 +775,7 @@ INIT_FUNCTION
 DisplayBootBitmap(IN BOOLEAN TextMode)
 {
    PVOID Header = NULL, Footer = NULL, Screen = NULL;
+    UCHAR Buffer[128];


Where does this 128-byte value comes from?

Since bar bitmap is 22x9 and 4bpp (hardcoded, yes), minimum value would be 22 * 9 * (4 bpp / 8 bits) = 99, but I made it a bit larger to reserve.

Also I don't know whether InbvScreenToBufferBlt uses some headers for the data.

It calls VidScreenToBufferBlt (in bootvid.dll).
By looking at the code, this function seems to use Delta * Height bytes in the buffer (delta is the last parameter of the function). I don't personally completely understand yet what this Delta is for.

The scan lines in a bitmap get aligned (to a 4-byte boundary IIRC), so the scanline width can be different from the image width. Delta is likely the scanline width.

@yagoulas commented in IRC:

it looks like delta is the width of the source bitmap
and I gues this would be used when you want to paint only a part of the marqee when it has reached the right part of the scrollbar and starts disappearing
this would be a case when width<Delta

The hypothesis of the scanline width makes sense, and this would explain the value of "4" (or "8", don't remember) seen in other places where this function is called, and where the used Delta is completely different than the specified Width.

Ok, so 24 * 9 * (4/8) = 108, still should be ok.

So, UCHAR Buffer[24 * 9 * 4 / 8];? ;)

Yeah, and it used in three places... 😆

HBelusca · 2017-12-14T14:33:21Z

ntoskrnl/inbv/inbv.c

+{
+    ULONG X, Y, Iteration, Index, Total = 18;
+    UCHAR Buffer[128];
+#ifndef INBV_ROTBAR_IMPLEMENTED


Instead of this you could do:

#ifdef INBV_ROTBAR_IMPLEMENTED VOID NTAPI ... InbvRotBarInit(VOID) { // The implementation return; } #else VOID NTAPI ... InbvRotBarInit(VOID) { return; } #endif

or even, just have the function defined in that case and calling in the main code only between #ifdef INBV_ROTBAR_IMPLEMENTED guards ?

HBelusca · 2017-12-14T14:34:14Z

ntoskrnl/inbv/inbv.c

+    if (RotBarSelection != RB_SQUARE_CELLS)
+        return;
+
+    /* This function needs to be in system thread, but it's not yet */


Do you mean "needs to be called by system thread" ?

This part of code should be in while () loop instead of for, and run in context of a separate thread.

For now it's executed sequentially, so the loop is limited to 6 rotations (for demonstration).

ThFabba · 2017-12-14T14:51:17Z

ntoskrnl/inbv/inbv.c

@@ -920,8 +930,14 @@ DisplayBootBitmap(IN BOOLEAN TextMode)
 #endif

 #ifdef INBV_ROTBAR_IMPLEMENTED
+        /* Save previous screen pixels to buffer */
+        InbvScreenToBufferBlt(Buffer, 0, 0, 22, 9, 24);


You probably can move this whole block inside the if (Bar) since saving restoring is unnecessary otherwise?

ThFabba · 2017-12-14T14:51:33Z

ntoskrnl/inbv/inbv.c

@@ -932,18 +948,67 @@ DisplayBootBitmap(IN BOOLEAN TextMode)
    }

 #ifdef INBV_ROTBAR_IMPLEMENTED
+    /* Hide simple progress bar if not used */
+    if (RotBarSelection != RB_PROGRESS_BAR)
+        ShowProgressBar = FALSE;


Braces around all blocks please.

ThFabba · 2017-12-14T14:57:21Z

You probably have neither file system nor registry available during this part of boot, so making it user-configurable is not exactly trivial.

HBelusca · 2017-12-14T16:29:26Z

ntoskrnl/inbv/inbv.c

@@ -768,6 +775,7 @@ INIT_FUNCTION
 DisplayBootBitmap(IN BOOLEAN TextMode)
 {
    PVOID Header = NULL, Footer = NULL, Screen = NULL;
+    UCHAR Buffer[128];


To be placed inside a #ifdef INBV_ROTBAR_IMPLEMENTED.

HBelusca · 2017-12-14T20:41:42Z

You're right Thomas; actually it seems the best way would be:

either via a parameter given to the kernel command-line (and specified via freeldr.ini entries, similar to the NOLOGO or NOGUIBOOT parameter).
patch ntoskrnl or something.

ThFabba · 2017-12-14T21:16:38Z

Yes, and even the parameter would likely not work, since, as I said, no filesystem access! You might need to do something like compile the logo as a resource into a sys file, have freeldr load that as a boot driver and then pass a kernel command line argument to specify that driver's name. In which case you might as well have recompiled the kernel in the first place... Like I said, nontrivial :)

ColinFinck · 2017-12-15T09:57:51Z

I remember that we had the rotation bar early in 0.3.1 times (just when the current bootvid was checked in). It was a very buggy implementation, so someone shortly disabled it. Later, the current progress bar was added.

Given that we have a progress bar that shows boot progress from 0 to 100%, I wonder what's the value of a less informative rotation bar? Just because XP does it? Well, Win2k had a progress bar too, and I preferred that :)

I'm all for making our boot animation more exciting. However, that should be more than just replacing the progress bar.

gigaherz · 2017-12-15T10:06:43Z

I think it's good to have some animation, because it shows that the system is not frozen. But it doesn't have to be XP's "marquee" bar. We could easily go the macOS way and show an actual spinner/throbber animation, if showing a full blown boot animation (like vista+ do) would be too much without boot-time graphics support. A small throbber animation should be fairly easy to produce and implement, and not slower than the not-really-progress bar?

gigaherz · 2017-12-15T10:09:00Z

Because I wasn't clear: I do agree with Colin that the existing progressbar should stay, I'm talking about an added thing to show that it's not frozen.

learn-more · 2017-12-15T10:16:01Z

Maybe some animantion that uses the progress bar like in Win7's progress bar?

https://social.msdn.microsoft.com/Forums/getfile/390525

gigaherz · 2017-12-15T10:19:49Z

That may be too slow depending on how large the progressbar gets. The idea of the small rotating image or a spinner, is that the number of pixels redrawn each frame is relatively small (if the drawing code only writes the needed pixels), which is important due to having to poke the video memory directly since we don't have boot-time video support like NT6 does.

EDIT: On second thought, if the XP-style animation was actually the highlight, contained in the "completed" area of a progressbar, that would work!

binarymaster · 2017-12-15T11:21:57Z

Please notice this PR is not targeting to replace current progress bar (I like it too) but to bring up an ability to change its behaviour at compile time (or maybe at boot time later).

When it will be finished, it would be simpler to create some other animations by reusing the code.

binarymaster · 2017-12-16T11:41:45Z

Rewrote part of drawing code, so now it properly renders appearing part from the left. 😉

Demonstration: https://webmshare.com/1WZNg

binarymaster · 2017-12-16T12:21:17Z

By looking at the code, this function seems to use Delta * Height bytes in the buffer

This was correct, the pixels are stored in bytes inside 8-bit buffer, so the size should be 24 * 9 and not 24 * 9 * 4 / 8, and this fixed the BSOD!

Also I noticed that InbvSolidColorFill uses InbvAcquireLock(); and InbvReleaseLock(); but the lock is already acquired before InbvRotBarInit, so I replaced it with VidSolidColorFill, that fixed the hang after animation!

So there is only one step left - to make it run separately (in thread) along with boot loading process. @ThFabba do you know how to do that?

HBelusca · 2017-12-16T13:53:16Z

About the locking thing, maybe there is a proper way to reorganize that (in your helper function) without the need to directly call the Vid* functions.
Otherwise the demo looks nice!
Any idea why it ends with a BSOD though?

ThFabba · 2017-12-16T13:58:34Z

I don't know if that's available at this point in the boot process, but normally to create a thread you would use PsCreateSystemThread, like here:
https://github.com/reactos/reactos/blob/master/modules/rostests/kmtests/include/kmt_test_kernel.h#L136-L187

binarymaster · 2017-12-16T14:01:07Z

Any idea why it ends with a BSOD though?

I already fixed it (commented above), this video demo is a bit outdated already :)

ThFabba · 2017-12-16T14:06:09Z

Directly calling the Vid* functions from inside Inbv seems correct -- in fact I'd say you probably should do it for VidBufferToScreenBlt as well.

HBelusca · 2017-12-27T21:31:19Z

Just for reference purposes, here is our (very) old animation procedure we used to have in bootvid: BootVidAnimationThread , before this module got rewritten.

HBelusca · 2017-12-27T22:31:56Z

Code updated with @ThFabba's suggestions.

HBelusca · 2017-12-27T22:36:17Z

ntoskrnl/inbv/inbv.c

            Status = PsCreateSystemThread(&ThreadHandle,
-                                          SYNCHRONIZE,
+                                          0,


The '0' works OK.

HBelusca · 2017-12-27T22:37:57Z

ntoskrnl/inbv/inbv.c

+    RB_ANIMATE,
+    RB_STOP_ANIMATE,
+    RB_STATUSMAX
+} ROT_BAR_STATUS;


If you have better names for all these, just let us know!

Maybe RBS_* ?

ThFabba · 2017-12-27T23:33:54Z

ntoskrnl/inbv/inbv.c

-    {
-        /* Acquire the lock */
-        InbvAcquireLock();
+    LARGE_INTEGER Delay; // Specified in nanoseconds


Not nanoseconds

Heh indeed, it's in hundreds of nanoseconds :) I'll fix the comments; the calculations/values used for Delay are however OK.

ThFabba · 2017-12-27T23:34:26Z

ntoskrnl/inbv/inbv.c

+    LARGE_INTEGER Delay = {{0}};
+
+    // FIXME: This was added to suppress thread hanging
+    KeSetPriorityThread(KeGetCurrentThread(), HIGH_PRIORITY);


I'd still like to get rid of this before merging

@binarymaster, could you please check whether things work without the priority setting? I think they should.

Ok, I retested without this change, and found no difference. So this can be removed.

binarymaster · 2017-12-28T06:46:15Z

Just for reference purposes, here is our (very) old animation procedure we used to have in bootvid: BootVidAnimationThread , before this module got rewritten.

Interesting... rotation bar animation was implemented with 3 buffers for each square cell :)

binarymaster · 2017-12-28T13:15:23Z

ntoskrnl/inbv/inbv.c

@@ -161,7 +161,7 @@ BootLogoFadeIn(VOID)
    LPRGBQUAD Palette = (LPRGBQUAD)(PaletteBitmapBuffer + sizeof(BITMAPINFOHEADER));
    ULONG Iteration, Index, ClrUsed;

-    LARGE_INTEGER Delay; // Specified in nanoseconds
+    LARGE_INTEGER Delay = {{0}}; // Specified in hundreds of nanoseconds


Does this needs to be initialized in all places, or it have only one field (QuadPart)?

I'm sure it zeroes out the Delay because when the while() loop is run for the first time, KeDelayExecutionThread is called with a valid delay.

Yes, specifying an initializer for any member of a struct, union, or array means the whole object will be initialized.
On an unrelated note, this comment seems completely unnecessary. The unit is obvious to anyone familiar with kernel programming, and anyone else needs to read the documentation for KeDelayExecutionThread anyway.

Ok, removed.

ThFabba · 2017-12-28T16:59:21Z

ntoskrnl/inbv/inbv.c

+            break;
+        }
+
+        if (PltRotBarStatus >= RBS_STATUS_MAX)


There's no way this can happen, right? So an ASSERT should be enough.

Added ASSERT.

ThFabba · 2017-12-28T17:00:31Z

ntoskrnl/inbv/inbv.c

+ */
+typedef enum _ROT_BAR_STATUS
+{
+    RBS_FADEIN = 1,


Is there a reason to make this distinct from 0?

Really, why isn't this just a boolean?

Originally (and as the original code -- before @binarymaster's patch -- also suggested via the value of "PltRotBarStatus"), this looked like it was designed as a "command" variable for the animation thread to know what to do "next", like, first, do the palette fading-in, then the rotation, then stop (in the usual order, but you can also choose to blank the screen again, then redo a fading in etc...).
In the current code the fading-in is done before starting the animation thread; I was wondering whether it could be nice instead to move it in the animation thread where that PltRotBarStatus variable would then make better sense (I didn't do the move as you can see).
But perhaps we can also do the way of the old bootvid code (see my link above), where the animation thread always do "fading in" first, then bar rotation, and thus the PltRotBarStatus would just become a Boolean "start / stop".

I'd prefer leave this variable as is...

ColinFinck · 2017-12-30T12:26:40Z

Judging from your latest demonstration video, you are using the rotation bar bitmap from Windows Server 2003. This must be changed to something custom should this PR be merged.
I remember that the original bootvid rotation bar code by Filip Navara used a custom bitmap that nicely integrated into the rest of the ReactOS boot screen. Maybe you can use that.

Nevertheless, I still prefer a boot screen with a progress bar that shows actual progress over a rotating one.
I'm not opposed to improving our boot animation and its code in general though.

binarymaster · 2017-12-30T12:34:56Z

@ColinFinck

Judging from your latest demonstration video, you are using the rotation bar bitmap from Windows Server 2003.

No, I haven't used it. I even didn't modify it (see diff), this bitmap is slightly different and currently in master branch.

Nevertheless, I still prefer a boot screen with a progress bar that shows actual progress over a rotating one.

Have you tried building my PR branch? 😈 There's already a different animation, which also stated in TODO list. 😉

binarymaster · 2017-12-31T11:22:25Z

https://gfycat.com/EvergreenBrilliantCoelacanth

New GIF demo ;)

HBelusca · 2017-12-31T19:08:44Z

@ThFabba and @ColinFinck , is everything OK so that we can proceed to a squash-and-merge?

CORE-10327 #resolve

- Use KeDelayExecutionThread() instead of KeStallExecutionProcessor(). - Fix magic values and add comments. - Fix structure name.

binarymaster · 2018-01-02T13:02:52Z

Rearranged commits for rebase and merge (two commits without squash), I think it would be better.

* Update openoffice.txt * Update SHA1 hash

sanchaez added help wanted Request for help. WIP labels Dec 14, 2017

HBelusca reviewed Dec 14, 2017

View reviewed changes

ThFabba reviewed Dec 14, 2017

View reviewed changes

binarymaster force-pushed the bootloader-bar branch from cd56569 to 08702a7 Compare December 14, 2017 15:12

HBelusca reviewed Dec 14, 2017

View reviewed changes

binarymaster force-pushed the bootloader-bar branch 2 times, most recently from 7d7ce5f to be981d8 Compare December 14, 2017 16:52

binarymaster force-pushed the bootloader-bar branch from be981d8 to 2839ba0 Compare December 16, 2017 11:31

binarymaster force-pushed the bootloader-bar branch from 2839ba0 to e6556ac Compare December 16, 2017 12:01

binarymaster force-pushed the bootloader-bar branch from e6556ac to 400a80c Compare December 16, 2017 17:03

HBelusca force-pushed the bootloader-bar branch from c872296 to 503c1b9 Compare December 27, 2017 22:31

HBelusca reviewed Dec 27, 2017

View reviewed changes

ntoskrnl/inbv/inbv.c Outdated

Status = PsCreateSystemThread(&ThreadHandle,

SYNCHRONIZE,

0,

Copy link

Contributor

HBelusca Dec 27, 2017

Choose a reason for hiding this comment

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

The '0' works OK.

HBelusca reviewed Dec 27, 2017

View reviewed changes

HBelusca force-pushed the bootloader-bar branch from 503c1b9 to 223bc38 Compare December 27, 2017 22:39

ThFabba reviewed Dec 27, 2017

View reviewed changes

binarymaster commented Dec 28, 2017

View reviewed changes

ThFabba reviewed Dec 28, 2017

View reviewed changes

binarymaster referenced this pull request Dec 28, 2017

[NTOS:INBV] Implement rotation bar for boot screen

57c0678

binarymaster force-pushed the bootloader-bar branch from 981c83a to 29a8b40 Compare December 28, 2017 18:16

binarymaster force-pushed the bootloader-bar branch from 29a8b40 to 63c765a Compare December 31, 2017 10:26

binarymaster force-pushed the bootloader-bar branch from 63c765a to 215033b Compare January 2, 2018 12:47

binarymaster and others added 2 commits January 2, 2018 15:55

[NTOS:INBV] Implement rotation bar for boot screen

823c87c

CORE-10327 #resolve

[NTOS:INBV] Simplify a little bit the BootLogoFadeIn() function

071685d

- Use KeDelayExecutionThread() instead of KeStallExecutionProcessor(). - Fix magic values and add comments. - Fix structure name.

binarymaster force-pushed the bootloader-bar branch from 215033b to 071685d Compare January 2, 2018 12:57

HBelusca removed the help wanted Request for help. label Jan 3, 2018

HBelusca merged commit b2be558 into reactos:master Jan 3, 2018

binarymaster deleted the bootloader-bar branch January 3, 2018 07:05

HBelusca added enhancement For PRs with an enhancement/new feature. and removed WIP labels Aug 1, 2019

julenuri pushed a commit to julenuri/reactos that referenced this pull request Dec 9, 2023

[RAPPS-DB] Update for AOO 4.1.14 release (reactos#194)

75a7699

* Update openoffice.txt * Update SHA1 hash

[NTOS:INBV] Implement rotation bar for boot screen #194

[NTOS:INBV] Implement rotation bar for boot screen #194

Conversation

binarymaster commented Dec 14, 2017 • edited

Purpose

Proposed changes

TODO

HBelusca commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBelusca Dec 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba commented Dec 14, 2017

Choose a reason for hiding this comment

HBelusca commented Dec 14, 2017

ThFabba commented Dec 14, 2017 • edited

ColinFinck commented Dec 15, 2017

gigaherz commented Dec 15, 2017

gigaherz commented Dec 15, 2017

learn-more commented Dec 15, 2017

gigaherz commented Dec 15, 2017 • edited

binarymaster commented Dec 15, 2017

binarymaster commented Dec 16, 2017

binarymaster commented Dec 16, 2017

HBelusca commented Dec 16, 2017 • edited

ThFabba commented Dec 16, 2017

binarymaster commented Dec 16, 2017

ThFabba commented Dec 16, 2017

HBelusca commented Dec 27, 2017 • edited

HBelusca commented Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binarymaster commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinFinck commented Dec 30, 2017

binarymaster commented Dec 30, 2017 • edited by sanchaez

binarymaster commented Dec 31, 2017

HBelusca commented Dec 31, 2017

binarymaster commented Jan 2, 2018

binarymaster commented Dec 14, 2017 •

edited

HBelusca Dec 14, 2017 •

edited

ThFabba commented Dec 14, 2017 •

edited

gigaherz commented Dec 15, 2017 •

edited

HBelusca commented Dec 16, 2017 •

edited

HBelusca commented Dec 27, 2017 •

edited

HBelusca commented Dec 27, 2017 •

edited

binarymaster commented Dec 30, 2017 •

edited by sanchaez