June 2022 - Wine-Devel - list.winehq.org

[tools] testbot/web: Add a task failures block to the failure details page.

by Francois Gouget

This allows reviewing all instances of a given failure. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48912 Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/web/FailureDetails.pl | 101 ++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/testbot/web/FailureDetails.pl b/testbot/web/FailureDetails.pl index c05146448..c510d8d48 100644 --- a/testbot/web/FailureDetails.pl +++ b/testbot/web/FailureDetails.pl @@ -19,6 +19,93 @@ use strict; + +package TaskFailuresBlock; + +use ObjectModel::CGI::CollectionBlock; +our @ISA = qw(ObjectModel::CGI::CollectionBlock); + +use ObjectModel::BasicPropertyDescriptor; +use ObjectModel::CGI::ValueFormatter; + +use WineTestBot::Jobs; +use WineTestBot::Utils; + + +sub GetPropertyDescriptors($) +{ + my ($self) = @_; + + # Add some extra columns + my @PropertyDescriptors; + foreach my $PropertyDescriptor (@{$self->{Collection}->GetPropertyDescriptors()}) + { + my $PropertyName = $PropertyDescriptor->GetName(); + next if ($PropertyName =~ /^(?:JobId|StepNo|TaskNo)$/); + push @PropertyDescriptors, $PropertyDescriptor; + if ($PropertyName eq "TaskLog") + { + push @PropertyDescriptors, CreateBasicPropertyDescriptor("Date", "Date", !1, !1, "DT", 19); + } + } + push @PropertyDescriptors, CreateBasicPropertyDescriptor("Remarks", "Remarks", !1, !1, "A", 128); + + return \@PropertyDescriptors; +} + +sub GenerateHeaderView($$$) +{ + my ($self, $Row, $Col) = @_; + + my $PropertyName = $Col->{Descriptor}->GetName(); + if ($PropertyName eq "NewCount" or $PropertyName eq "OldCount") + { + $PropertyName =~ s/Count$//; + print "<a class='title' title='", $self->escapeHTML($Col->{Descriptor}->GetDisplayName()), "'>$PropertyName</a>\n"; + } + else + { + $self->SUPER::GenerateHeaderView($Row, $Col); + } +} + +sub GenerateDataView($$$) +{ + my ($self, $Row, $Col) = @_; + + my $PropertyName = $Col->{Descriptor}->GetName(); + if ($PropertyName eq "Task") + { + my $JobId = $Row->{Item}->JobId; + my $StepNo = $Row->{Item}->StepNo; + my $TaskNo = $Row->{Item}->TaskNo; + my $URL = GetTaskURL($JobId, $StepNo, $TaskNo); + print "<a href='", $self->escapeHTML($URL), "'>$JobId/$StepNo/$TaskNo</a>"; + } + elsif ($PropertyName eq "TaskLog") + { + my $URL = GetTaskURL($Row->{Item}->JobId, $Row->{Item}->StepNo, + $Row->{Item}->TaskNo, 0, $Row->{Item}->TaskLog); + print "<a href='", $self->escapeHTML($URL), "'>", + $self->escapeHTML($Row->{Item}->TaskLog), "</a>"; + } + elsif ($PropertyName eq "Date") + { + # The enclosing page already pulled in the datetime JavaScript code + GenerateDateTime($Row->{Item}->Task->Started); + } + elsif ($PropertyName eq "Remarks") + { + my $Job = CreateJobs()->GetItem($Row->{Item}->JobId); + print $self->escapeHTML($Job->Remarks); + } + else + { + $self->SUPER::GenerateDataView($Row, $Col); + } +} + + package FailureDetailsPage; use ObjectModel::CGI::ItemPage; @@ -71,6 +158,20 @@ sub GenerateValueView($$$$) } } +sub GenerateBody($) +{ + my ($self) = @_; + + $self->SUPER::GenerateBody(); + if (!$self->{Item}->GetIsNew()) + { + print "<h2><a name='TaskFailures'></a>Task Failures</h2>\n"; + my $TaskFailuresBlock = new TaskFailuresBlock($self->{Item}->TaskFailures, $self); + $TaskFailuresBlock->SetReadWrite(0) if (!$self->{RW}); + $TaskFailuresBlock->GenerateList(); + } +} + sub GenerateFooter($) { my ($self) = @_; -- 2.30.2

June 16, 2022

[tools] testbot: Better track job status changes.

by Francois Gouget

Only set OldStatus once to avoid issues in case the status is changed multiple times before the job is saved. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/lib/WineTestBot/Jobs.pm | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm index 3da6ae7b0..b54f864da 100644 --- a/testbot/lib/WineTestBot/Jobs.pm +++ b/testbot/lib/WineTestBot/Jobs.pm @@ -92,15 +92,6 @@ use WineTestBot::Engine::Notify; use WineTestBot::TaskFailures; -sub _initialize($$) -{ - my ($self, $Collection) = @_; - - $self->SUPER::_initialize($Collection); - - $self->{OldStatus} = undef; -} - sub InitializeNew($$) { my ($self, $Collection) = @_; @@ -171,10 +162,10 @@ sub Status($;$) my $CurrentStatus = $self->SUPER::Status; return $CurrentStatus if (!defined $NewStatus); - if (! defined($CurrentStatus) || $NewStatus ne $CurrentStatus) + if (!defined $CurrentStatus or $NewStatus ne $CurrentStatus) { + $self->{OldStatus} ||= $CurrentStatus; $self->SUPER::Status($NewStatus); - $self->{OldStatus} = $CurrentStatus; } return $NewStatus; @@ -185,15 +176,11 @@ sub OnSaved($) my ($self) = @_; $self->SUPER::OnSaved(); - - if (defined($self->{OldStatus})) + if (defined $self->{OldStatus} and $self->{OldStatus} ne $self->Status) { - my $NewStatus = $self->Status; - if ($NewStatus ne $self->{OldStatus}) - { - JobStatusChange($self->GetKey(), $self->{OldStatus}, $NewStatus); - } + JobStatusChange($self->GetKey(), $self->{OldStatus}, $self->Status); } + delete $self->{OldStatus}; } =pod -- 2.30.2

June 16, 2022

[tools] patches: Take the .testbot file Status into account.

by Francois Gouget

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- So far whether a patch passed the tests or not seems to only be based on the wine-devel emails, despite the information being available in the locale .testbot file. --- patches/update | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/patches/update b/patches/update index 5d20f2a11..494d2e157 100755 --- a/patches/update +++ b/patches/update @@ -210,7 +210,8 @@ foreach my $file (readdir DIR) { utf8::decode($_); last if /^$/; - if (/^Job-ID: (.*)$/) { $patch{"testjob"} = $1; } + if (/^Status: (.*)$/) { $patch{"testbot"} = $1; } + elsif (/^Job-ID: (.*)$/) { $patch{"testjob"} = $1; } elsif (/^URL: (.*)$/) { $patch{"testurl"} = $1; } } close LOG; @@ -254,7 +255,6 @@ sub assign_parent($) if ($patch->{"from_marvin"}) { $parent->{"testbot"} = "<a href=\"data/$patch->{id}\">Failed</a>"; - $parent->{"testclass"} = "testbot botfail"; } return $parent; } @@ -320,6 +320,10 @@ foreach my $file (sort { $patches{$b}->{"order"} <=> $patches{$a}->{"order"} } k { printf INDEX "<td class=\"failmark\">X</td>"; } + if ($patch->{"testbot"} and $patch->{"testbot"} ne "OK") + { + $patch->{"testclass"} .= " botfail"; + } printf INDEX "<td class=\"%s\">%s</td>", $patch->{"testclass"}, $patch->{"testbot"}; if ($patch->{"testjob"} && $patch->{"testurl"}) { -- 2.30.2

June 16, 2022

[tools] testbot/web: Tweak the failures page columns.

by Francois Gouget

Show the bug id in the failure list: this makes it easier to find all the failures related to a given bug. Shorten some column labels so they don't make the column too wide. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/lib/WineTestBot/Failures.pm | 6 +++--- testbot/web/FailuresList.pl | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/testbot/lib/WineTestBot/Failures.pm b/testbot/lib/WineTestBot/Failures.pm index a9118f64b..f0f78b6ff 100644 --- a/testbot/lib/WineTestBot/Failures.pm +++ b/testbot/lib/WineTestBot/Failures.pm @@ -187,9 +187,9 @@ my @PropertyDescriptors = ( # Every entry must be associated with a Wine bug. # Note: The 'deleted' bug status means this failure entry should be # deleted as soon as it is not referenced anymore. - CreateBasicPropertyDescriptor("BugId", "WineHQ bug id", !1, 1, "N", 10), - CreateBasicPropertyDescriptor("BugStatus", "WineHQ bug status", !1, !1, "A", 32), - CreateBasicPropertyDescriptor("BugDescription", "WineHQ bug description", !1, !1, "A", 128), + CreateBasicPropertyDescriptor("BugId", "Bug id", !1, 1, "N", 10), + CreateBasicPropertyDescriptor("BugStatus", "Bug status", !1, !1, "A", 32), + CreateBasicPropertyDescriptor("BugDescription", "WineHQ Bug description", !1, !1, "A", 128), CreateDetailrefPropertyDescriptor("TaskFailures", "Tasks", \&CreateTaskFailures), ); diff --git a/testbot/web/FailuresList.pl b/testbot/web/FailuresList.pl index 8825fea69..e271ee1d5 100644 --- a/testbot/web/FailuresList.pl +++ b/testbot/web/FailuresList.pl @@ -55,7 +55,7 @@ sub DisplayProperty($$) my ($self, $PropertyDescriptor) = @_; my $PropertyName = $PropertyDescriptor->GetName(); - return $PropertyName =~ /^(?:Notes|ConfigRegExp|FailureRegExp|BugId)$/ ? "" : + return $PropertyName =~ /^(?:Notes|ConfigRegExp|FailureRegExp)$/ ? "" : $self->SUPER::DisplayProperty($PropertyDescriptor); } @@ -79,7 +79,7 @@ sub GenerateDataView($$$) my ($self, $Row, $Col) = @_; my $PropertyName = $Col->{Descriptor}->GetName(); - if ($PropertyName =~ /^Test(?:Module|Unit)$/) + if ($PropertyName eq "TestUnit") { print "<a href='", $self->escapeHTML($self->GetDetailsLink($Row)), "'>", $self->escapeHTML($Row->{Item}->$PropertyName), "</a>"; -- 2.30.2

June 16, 2022

[tools 2/2] testbot/Engine: Also handle notifications originating from the Engine.

by Francois Gouget

In particular notifications of job status changes must be processed in order for the result emails to be sent and for the patch status to be updated. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- I believe this explains why a number of developer emails were getting lost during some tests I ran. I had suspected ISP-level spam filtering issues, or too big emails, but I recently noticed cases where WineSendLog was not getting called which turned out to be because of this issue. --- testbot/bin/Engine.pl | 2 +- testbot/lib/WineTestBot/Engine/Notify.pm | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index 29dce183b..9d836529c 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -751,7 +751,7 @@ sub main() $SIG{CHLD} = \&REAPER; WineTestBot::Log::SetupRedirects(); - $WineTestBot::Engine::Notify::RunningInEngine = 1; + SetNotificationHandler(\&HandleClientCmd); LogMsg "Starting the WineTestBot Engine\n"; # Validate and adjust the configuration options diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm index 9c1802984..c9930e6e6 100644 --- a/testbot/lib/WineTestBot/Engine/Notify.pm +++ b/testbot/lib/WineTestBot/Engine/Notify.pm @@ -2,6 +2,7 @@ # Notification of WineTestBot engine # # Copyright 2009 Ge van Geldorp +# Copyright 2012-2022 Francois Gouget # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -29,8 +30,8 @@ WineTestBot::Engine::Notify - Engine notification use Exporter 'import'; our $RunningInEngine; -our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl - RescheduleJobs VMStatusChange +our @EXPORT = qw(SetNotificationHandlerShutdown PingEngine JobStatusChange + JobControl RescheduleJobs VMStatusChange WinePatchMLSubmission WinePatchWebSubmission UpdateFailure GetScreenshot); our @EXPORT_OK = qw($RunningInEngine); @@ -38,14 +39,21 @@ our @EXPORT_OK = qw($RunningInEngine); use Socket; use WineTestBot::Config; +my $_CmdHandler; + +sub SetNotificationHandler($) +{ + my ($Handler) = @_; + $_CmdHandler = $Handler; +} sub SendCmdReceiveReply($) { my ($Cmd) = @_; - if (defined($RunningInEngine)) + if ($_CmdHandler) { - return "1"; + return &$_CmdHandler(split ' ', $Cmd); } if (! socket(SOCK, PF_UNIX, SOCK_STREAM, 0)) -- 2.30.2

June 16, 2022

[tools 1/2] testbot/web: Automatically refresh the failure bug information.

by Francois Gouget

Automatically refresh the failure bug information when the bug id is modified. Also trigger a refresh when a failure is undeleted. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48912 Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- This ensure failures have an initial bug status and description as soon as they are added. Later refreshes happen when UpdateFailures is called by cron. --- testbot/bin/Engine.pl | 39 ++++++++++++++++++++++++ testbot/lib/WineTestBot/Engine/Notify.pm | 20 +++++++++++- testbot/lib/WineTestBot/Failures.pm | 31 +++++++++++++++++++ testbot/web/FailureDetails.pl | 1 - testbot/web/FailuresList.pl | 5 ++- 5 files changed, 91 insertions(+), 5 deletions(-) diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index 88852730e..29dce183b 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -49,6 +49,7 @@ use WineTestBot::Config; use WineTestBot::Engine::Events; use WineTestBot::Engine::Notify; use WineTestBot::Engine::Scheduler; +use WineTestBot::Failures; use WineTestBot::Jobs; use WineTestBot::Log; use WineTestBot::Patches; @@ -548,6 +549,43 @@ sub HandleGetScreenshot($) return "1" . $ImageBytes; } +sub HandleUpdateFailure($) +{ + my ($FailureId) = @_; + + # Validate FailureId + if ($FailureId !~ /^(\d+)$/) + { + LogMsg "Invalid failure id\n"; + return "0Invalid Failure Id"; + } + $FailureId = $1; + + if (!CreateFailures()->GetItem($FailureId)) + { + LogMsg "Unknown Failure $FailureId\n"; + return "0Unknown Failure $FailureId"; + } + + my $Pid = fork; + if (!defined $Pid) + { + LogMsg "Unable to fork for UpdateFailures.pl: $!\n"; + } + elsif (!$Pid) + { + # Clean up the child environment + CloseAllDBBackEnds(); + WineTestBot::Log::SetupRedirects(); + + exec("$BinDir/UpdateFailures.pl --log-only $FailureId") or + LogMsg "Unable to exec ${ProjectName}UpdateFailures.pl: $!\n"; + exit(1); + } + + return "1"; +} + my %Handlers=( "getscreenshot" => \&HandleGetScreenshot, "jobcontrol" => \&HandleJobControl, @@ -558,6 +596,7 @@ my %Handlers=( "vmstatuschange" => \&HandleVMStatusChange, "winepatchmlsubmission" => \&HandleWinePatchMLSubmission, "winepatchwebsubmission" => \&HandleWinePatchWebSubmission, + "updatefailure" => \&HandleUpdateFailure, ); sub HandleClientCmd(@) diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm index 82221c9ed..9c1802984 100644 --- a/testbot/lib/WineTestBot/Engine/Notify.pm +++ b/testbot/lib/WineTestBot/Engine/Notify.pm @@ -31,7 +31,8 @@ use Exporter 'import'; our $RunningInEngine; our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl RescheduleJobs VMStatusChange - WinePatchMLSubmission WinePatchWebSubmission GetScreenshot); + WinePatchMLSubmission WinePatchWebSubmission + UpdateFailure GetScreenshot); our @EXPORT_OK = qw($RunningInEngine); use Socket; @@ -194,6 +195,23 @@ sub WinePatchWebSubmission() return substr($Reply, 1); } +sub UpdateFailure($) +{ + my ($FailureId) = @_; + + my $Reply = SendCmdReceiveReply("updatefailure $FailureId\n"); + if (length($Reply) < 1) + { + return "The Engine did not acknowledge the UpdateFailure command"; + } + if (substr($Reply, 0, 1) eq "1") + { + return (undef, substr($Reply, 1)); + } + + return (substr($Reply, 1), undef); +} + sub GetScreenshot($) { my ($VMName) = @_; diff --git a/testbot/lib/WineTestBot/Failures.pm b/testbot/lib/WineTestBot/Failures.pm index b37b9e046..a9118f64b 100644 --- a/testbot/lib/WineTestBot/Failures.pm +++ b/testbot/lib/WineTestBot/Failures.pm @@ -40,6 +40,8 @@ TaskFailure objects. use WineTestBot::WineTestBotObjects; our @ISA = qw(WineTestBot::WineTestBotItem); +use WineTestBot::Engine::Notify; + sub Compare($$) { @@ -55,6 +57,35 @@ sub Compare($$) return $Cmp } +sub BugId($;$) +{ + my ($self, $NewBugId) = @_; + + my $CurrentBugId = $self->SUPER::BugId; + return $CurrentBugId if (!defined $NewBugId); + + if (!defined $CurrentBugId or $NewBugId ne $CurrentBugId) + { + $self->{OldBugId} = $CurrentBugId if (!defined $self->{OldBugId}); + $self->SUPER::BugId($NewBugId); + } + + return $NewBugId; +} + +sub OnSaved($) +{ + my ($self) = @_; + + $self->SUPER::OnSaved(); + if ((defined $self->{OldBugId} and $self->{OldBugId} ne $self->BugId) or + !$self->BugStatus or !$self->BugDescription) + { + UpdateFailure($self->Id); + } + delete $self->{OldBugId}; +} + sub Validate($) { my ($self) = @_; diff --git a/testbot/web/FailureDetails.pl b/testbot/web/FailureDetails.pl index 7cab1d2a6..c05146448 100644 --- a/testbot/web/FailureDetails.pl +++ b/testbot/web/FailureDetails.pl @@ -105,7 +105,6 @@ sub OnAction($$) $self->{Item}->BugDescription(""); } return undef if (!$self->Save()); - # FIXME Notify the TestBot Engine so it updates the bug information exit($self->RedirectToParent()); } return $self->SUPER::OnAction($Action); diff --git a/testbot/web/FailuresList.pl b/testbot/web/FailuresList.pl index f2608dc45..8825fea69 100644 --- a/testbot/web/FailuresList.pl +++ b/testbot/web/FailuresList.pl @@ -129,9 +129,9 @@ sub OnItemAction($$$) return 1 if ($Action eq "Restore" and $Failure->BugStatus ne "deleted"); my $NewStatus = $Action eq "Delete" ? "deleted" : - $Action eq "Restore" ? "unknown" : + $Action eq "Restore" ? "" : undef; - if ($NewStatus) + if (defined $NewStatus) { $Failure->BugStatus($NewStatus); my ($_ErrProperty, $ErrMessage) = $Failure->Save(); @@ -141,7 +141,6 @@ sub OnItemAction($$$) $self->{EnclosingPage}->SetError(undef, $ErrMessage); return 0; } - # FIXME Notify the TestBot Engine so it updates the bug information return 1; } } -- 2.30.2

June 16, 2022

Re: [PATCH v3 7/8] mflat/buffer: Support YV12, I420 and IYUV image formats.

by Giovanni Mascellani

Hi, Il 16/06/22 14:51, Nikolay Sivov ha scritto: > We should figure out better tests for 2/8 first. According to msdn > description IYUV and YV12 are similar, but IMC2/4 are using different > strides. My understanding is that they're using the same strides, they're just organizing UV data in a different way. Specifically, IMC2 has a half-stride of U data, than a half-stride of V data, and it keeps interleaving. YV12 has all the V data in half-strides and then all the U data in half-strides. They occupy the same exact bytes, but with the U and V planes interleaved in the case of IMC2 and kept together in the case of YV12 (and then IMC4 and IYUV are just the same, except swapping U and V). Giovanni.

June 16, 2022

Re: [PATCH v3 2/8] mfplat/buffer: Fix image copy function for IMC2/IMC4 buffers with odd height.

by Giovanni Mascellani

Hi, Il 16/06/22 14:33, Nikolay Sivov ha scritto: > I don't understand this. Why would it use fractions of strides? And > doing one copy instead of two related to odd heights? Or does the > subject need adjusting? Yeah, maybe it's better to explain; first, notice that when the height is even mine code and yours are precisely equivalent: copying x/2 lines of data with stride y and then other x/2 with the same stride y, but offsetted by a y/2, is precisely equivalent to copying x lines with stride y/2. For example, suppose that width == 4, stride == 8 and lines == 4. You may represent the UV part of the image in this way (where "." denotes padding data): XX..YY.. XX..YY.. If you use the approach currently in the code, you will first copy all the X data and then all the Y data. But the same data can also be represented as XX.. YY.. XX.. YY.. I.e., with the double number of lines and half the stride. The code I am proposing "looks" at data in this way and does the copy with a single call to MFCopyImage(). Looking from this point of view, the change is correct, but not particularly useful. The point is that for odd heights the two approaches are different, and from my tests it seems that mine is correct. Specifically, if width == 4, stride == 8 and lines == 5 the data will look like: XX..YY.. XX..YY.. XX.. The current code will only copy the first two XX blocks (because it copies lines / 2 == 2 lines), but I think that Microsoft thinks that the third XX line is still part of the image. The new code will correctly copy all the five lines. Does this make sense? > >> static inline struct buffer *impl_from_IMFMediaBuffer(IMFMediaBuffer >> *iface) >> diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c >> index 1b5e8cf7ed5..52608c05929 100644 >> --- a/dlls/mfplat/tests/mfplat.c >> +++ b/dlls/mfplat/tests/mfplat.c >> @@ -6027,12 +6027,11 @@ static void test_MFCreate2DMediaBuffer(void) >> case MAKEFOURCC('I','M','C','2'): >> case MAKEFOURCC('I','M','C','4'): >> - for (j = ptr->height; j < length2 / stride; j++) >> + for (j = 0; ptr->height * stride + j * (stride / 2) < >> length2; j++) >> for (k = 0; k < ptr->width / 2; k++) >> - ok(data[j * pitch + k] == 0xff, "Unexpected >> byte %02x at test %d row %d column %d.\n", data[j * pitch + k], i, j, k); >> - for (j = ptr->height; j < length2 / stride; j++) >> - for (k = pitch / 2; k < pitch / 2 + ptr->width / >> 2; k++) >> - ok(data[j * pitch + k] == 0xff, "Unexpected >> byte %02x at test %d row %d column %d.\n", data[j * pitch + k], i, j, k); >> + ok(data[ptr->height * pitch + j * (pitch / 2) >> + k] == 0xff, >> + "Unexpected byte %02x at test %d row >> %d column %d.\n", >> + data[ptr->height * pitch + j * (pitch >> / 2) + k], i, j, k); >> break; > > It feels like this could be more readable. I see initial contents are > set to 0xff, so I'm not even sure what we are testing. What else could > it be if not 0xff? Yeah, the image format is a bit involved so I have to do some trickery to do the right checks. The point is that what we set of 0xff is the buffer locked with _Lock(), therefore in its contiguous format. When we then lock it with _Lock2D() there will be a lot of padding bytes that are not necessarily set to 0xff (though they could be, I don't think they're guaranteed to be zero). What that test is checking is which bytes of the 2D buffer are really part of the image (i.e., they're not padding), and the result is that the last half-line of the UV section of a image of odd height is still considered part of the image (therefore the change made in the first hunk is appropriate). You can try to keep the second hunk and revert the first one, and you'll see that the tests fail on Wine (but they don't on Windows). Thanks, Giovanni.

June 16, 2022

Re: [PATCH v3 7/8] mflat/buffer: Support YV12, I420 and IYUV image formats.

by Nikolay Sivov

We should figure out better tests for 2/8 first. According to msdn description IYUV and YV12 are similar, but IMC2/4 are using different strides.

June 16, 2022

Re: [PATCH v3 5/8] mfplat/buffer: Redirect all Lock2D() implementations to Lock2DSize().

by Nikolay Sivov

I think equivalent of memory_2d_buffer_lock() would be better. With same arguments, locking and arguments checks in methods.

June 16, 2022