[haiku-commits] Re: haiku: hrev49503 - src/kits/media src headers/os/media

  • From: Stephan Aßmus <superstippi@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Mon, 3 Aug 2015 11:08:45 +0200

Hi Dario,

some comments for this commit:

Am 03.08.2015 um 02:16 schrieb b.vitruvio@xxxxxxxxx:

############################################################################

Commit: 75b9de169f3cd4aa9a35e12f92f347a534398476
URL: http://cgit.haiku-os.org/haiku/commit/?id=75b9de169f3c
Author: Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
Date: Sun Aug 2 23:05:12 2015 UTC

MediaDefs: Fix percentage in when notifying the system

* Use TRACE instead of PRINT.

----------------------------------------------------------------------------

diff --git a/src/kits/media/MediaDefs.cpp b/src/kits/media/MediaDefs.cpp
index cd6e95a..614e89f 100644
--- a/src/kits/media/MediaDefs.cpp
+++ b/src/kits/media/MediaDefs.cpp
@@ -1261,7 +1261,7 @@ progress_shutdown(int stage,
// parameter "message" is no longer used. It is kept for compatibility
with
// BeOS as this is used as a shutdown_media_server callback.

- PRINT(("stage : %i\n", stage));
+ TRACE("stage : %i\n", stage);
const char* string = "Unknown stage";
switch (stage) {
case 10:
@@ -1285,7 +1285,7 @@ progress_shutdown(int stage,
}

if (progress == NULL)
- notify_system(stage, string);
+ notify_system(stage/100.0f, string);

Coding style: spaces around "/"


else
progress(stage, string, cookie);
}
@@ -1365,14 +1365,14 @@ progress_startup(int stage,
// parameter "message" is no longer used. It is kept for compatibility
with
// BeOS as this is used as a shutdown_media_server callback.

- PRINT(("stage : %i\n", stage));
+ TRACE("stage : %i\n", stage);
const char* string = "Unknown stage";
switch (stage) {
case 10:
string = B_TRANSLATE("Stopping media server"
B_UTF8_ELLIPSIS);
break;
case 20:
- string = B_TRANSLATE("Telling media_addon_server to
quit.");
+ string = B_TRANSLATE("Stopping media_addon_server.");
break;
case 50:
string = B_TRANSLATE("Starting media_services.");
@@ -1386,7 +1386,7 @@ progress_startup(int stage,
}

if (progress == NULL)
- notify_system(stage, string);
+ notify_system(stage/100.0f, string);

Coding style: spaces around "/"


else
progress(stage, string, cookie);
}

############################################################################

Commit: 313ed73b5052aa56878487ee050095659542b1dd
URL: http://cgit.haiku-os.org/haiku/commit/?id=313ed73b5052
Author: Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
Date: Sat Aug 1 12:08:44 2015 UTC

Revert "media_server: Fix deadlock when killing from ProcessController"

This reverts commit 5934b30b95a239eee4a088a9b9cb425eef13f22d.

* It got the way of an heisenbug and it seems this change
fixed the problems, but after 2 days the media_server returned
to block. Sorry about that! Will investigate further.

----------------------------------------------------------------------------

diff --git a/src/servers/media/media_server.cpp
b/src/servers/media/media_server.cpp
index 6fd937f..241c114 100644
--- a/src/servers/media/media_server.cpp
+++ b/src/servers/media/media_server.cpp
@@ -918,13 +918,6 @@ ServerApp::_ControlThread(void* _server)
int32 code;
while ((size = read_port_etc(server->_ControlPort(), &code, data,
sizeof(data), 0, 0)) > 0) {
-
- // NOTE: This prevents locks in Deskbar and
- // possibly in other situations.
- if (size == B_WOULD_BLOCK) {
- snooze(100);
- continue;
- }

Good that you removed this. Should not have been added in the first place.


############################################################################

Commit: 8ecc32df1138c4468af5abdfd07a5bcb528a312e
URL: http://cgit.haiku-os.org/haiku/commit/?id=8ecc32df1138
Author: Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
Date: Sun Aug 2 22:56:18 2015 UTC

BBufferProducer: Returning B_ERROR hang up things

----------------------------------------------------------------------------

diff --git a/src/kits/media/BufferProducer.cpp
b/src/kits/media/BufferProducer.cpp
index 2540a23..3358a7b 100644
--- a/src/kits/media/BufferProducer.cpp
+++ b/src/kits/media/BufferProducer.cpp
@@ -275,7 +275,7 @@ BBufferProducer::HandleMessage(int32 message, const void*
data, size_t size)
ERROR("BBufferProducer::HandleMessage
PRODUCER_SET_BUFFER_GROUP"
" group InitCheck() failed.\n");
delete group;
- return B_ERROR;
+ return B_OK;

Huh? Please revert. Either the git-context here is misleading, and this is actually not an error (please fix the tracing line then), or you need to fix other code elsewhere. You never improve code by doing something obviously wrong, just because it helps with one particular test you might be focussing on.


############################################################################

Revision: hrev49503
Commit: 7771139cdf652d3712c0b96b8e9e2dda6db6191e
URL: http://cgit.haiku-os.org/haiku/commit/?id=7771139cdf65
Author: Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
Date: Sun Aug 2 23:35:09 2015 UTC

BMediaEventLooper: Rewrite ControlLoop()

I don't agree with this commit. You have replaced code that was cleanly readable and made sense, with something very ugly (including the use of a goto) that does not make nearly as much sense as before. You even misuse variables like "lateness" which should be a delta, in whichever way you feel. Some comments below:


* The first problem was the O(n^2) complexity of the algorithm, it's
now linear and try to act in a circular way by dispatching
events and reading the port in a balanced way. This exclude
a certain degree of possible deadlocks.

Can you explain why that is the case? I don't see it in the code.


* Add detection and escape when the system try to kill the
thread. This solve some blocking issues on exit et similia
that i had with libjackcompat.

You mean the error-check on WaitForMessage()? That seems a good change.

* The algorithm choose soon which event to focus on.

That seems to be the essence, but I think you could have accomplished this in a much clearer why by changing the original loop in a simpler way.

* Lateness is calculated just before the event is dispatched
as it is the more appropriate place, otherwise we would be
calculating something imprecise/guessed.

I'm not sure the calculations for lateness are sound, since you initially assign "waitUntil" to lateness and then use it in non-obvious ways.

* Remove timed_event_queue::queued_time. It's more precise to
just use the RealTime() before to Dispatch the event.

I don't agree. Now you lost the information when you queued this event, so how do you know how late it is when you process it?

* It should solve the BSoundPlayer lateness problems.
* With those improvements the media_kit is not going to lock
completely under stress conditions, instead it try to work
in a best effort shape.
* There's still room for improvements, for example i'm considering some
strategies in lateness situations such as update scheduling latency,
try to decrease waiting time and detect when we are too early on
the other hand to recover when the load go down.
* Thanks to Julian Harnath for sharing his WIP patch which helped
with some controls such as avoiding negative lateness.
* Comments are welcome!

See below...

----------------------------------------------------------------------------

diff --git a/headers/os/media/TimedEventQueue.h
b/headers/os/media/TimedEventQueue.h
index 190a792..732fd17 100644
--- a/headers/os/media/TimedEventQueue.h
+++ b/headers/os/media/TimedEventQueue.h
@@ -38,9 +38,8 @@ struct media_timed_event {
int32 data;
int64 bigdata;
char user_data[64];
- bigtime_t queued_time; // Real
time when put in queue

- uint32
_reserved_media_timed_event_[6];
+ uint32
_reserved_media_timed_event_[8];
};


diff --git a/src/kits/media/MediaEventLooper.cpp
b/src/kits/media/MediaEventLooper.cpp
index 0ce44f5..d072bc3 100644
--- a/src/kits/media/MediaEventLooper.cpp
+++ b/src/kits/media/MediaEventLooper.cpp
@@ -1,4 +1,5 @@
/*
+ * Copyright (c) 2015 Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
* Copyright (c) 2002, 2003 Marcus Overhagen <Marcus@xxxxxxxxxxxx>
*
* Permission is hereby granted, free of charge, to any person obtaining
@@ -212,68 +213,71 @@ BMediaEventLooper::ControlLoop()
{
CALLED();

- bool is_realtime = false;
status_t err;
- bigtime_t latency;
- bigtime_t waituntil;
- bigtime_t lateness;
- for (;;) {
- // while there are no events or it is not time for the earliest
event,
- // process messages using WaitForMessages. Whenever this
funtion times out,
- // we need to handle the next event
- for (;;) {
- if (RunState() == B_QUITTING)
- return;
- // BMediaEventLooper compensates your performance time
by adding the event latency
- // (see SetEventLatency()) and the scheduling latency
(or, for real-time events,
- // only the scheduling latency).
-
- latency = fEventLatency + fSchedulingLatency;
- waituntil = B_INFINITE_TIMEOUT;
- if (fEventQueue.HasEvents()) {
- const media_timed_event *firstEvent =
fEventQueue.FirstEvent();
- waituntil =
TimeSource()->RealTimeFor(firstEvent->event_time, latency);
- is_realtime = false;
- lateness = firstEvent->queued_time - waituntil;
- if (lateness > 0) {
-// if (lateness > 1000)
-// printf("node %02ld handling %12Ld at
%12Ld -- %Ld late, queued at %Ld now %12Ld \n",
-// ID(),
fEventQueue.FirstEventTime(), TimeSource()->Now(), lateness,
-// firstEvent->queued_time,
TimeSource()->RealTime());
- is_realtime = false;
- break;
- }
-// printf("node %02ld waiting for %12Ld that will
happen at %12Ld\n", ID(), fEventQueue.FirstEventTime(), waituntil);
- }
- if (fRealTimeQueue.HasEvents()) {
- const media_timed_event *firstEvent =
fRealTimeQueue.FirstEvent();
- bigtime_t temp;
- temp = firstEvent->event_time -
fSchedulingLatency;
- lateness = firstEvent->queued_time - temp;

"queued_time" obviously made sense. When you have a real-time event, it seems importent for the lateness to know when it was scheduled/queued.

- if (lateness > 0) {
- is_realtime = true;
- break;
- }
- if (temp < waituntil) {
- waituntil = temp;
- is_realtime = true;
- }
- }
- lateness = 0; // remove any extraneous value if we
get this far
- err = WaitForMessage(waituntil);
- if (err == B_TIMED_OUT)
- break;
+ bigtime_t waitUntil = 0;
+ bigtime_t lateness = 0;
+ bool hasRealtime = false;
+ bool hasEvent = false;
+
+ // While there are no events or it is not time for the earliest event,
+ // process messages using WaitForMessages. Whenever this funtion times
out,
+ // we need to handle the next event
+
+ fSchedulingLatency = estimate_max_scheduling_latency(fControlThread);
+ while (true) {
+ if (RunState() == B_QUITTING)
+ return;
+
+ // BMediaEventLooper compensates your performance time by adding
+ // the event latency (see SetEventLatency()) and the scheduling
+ // latency (or, for real-time events, only the scheduling
latency).
+
+ waitUntil = B_INFINITE_TIMEOUT;
+ hasRealtime = fRealTimeQueue.HasEvents();
+ hasEvent = fEventQueue.HasEvents();
+
+ if (hasEvent) {
+ waitUntil = TimeSource()->RealTimeFor(
+ fEventQueue.FirstEvent()->event_time,
+ fEventLatency + fSchedulingLatency);
+ lateness = waitUntil;

lateness should be a delta, why assign it here at all?

+ } else if (!hasEvent && !hasRealtime)
+ goto ahead;

You don't need to check for "!hasEvent" in the else path of "if (hasEvent)".

+
+ if (hasEvent && hasRealtime) {
+ if (fRealTimeQueue.FirstEventTime()
+ - fSchedulingLatency <= waitUntil) {
+ hasEvent = false;
+ } else
+ hasRealtime = false;
}

What is the above trying to accomplish?


+ if (hasRealtime) {
+ waitUntil = fRealTimeQueue.FirstEventTime()
+ - fSchedulingLatency;
+ lateness = waitUntil;
+ }

Now it looks like you are overwriting something you already computed above, since it is so non-obvious which block of code is actually executed.


+ if (waitUntil <= TimeSource()->RealTime())
+ waitUntil = 0;
+
+ahead:
+ err = WaitForMessage(waitUntil);
+ if (err == B_TIMED_OUT) {
+ media_timed_event event;
+ if (hasEvent)
+ err = fEventQueue.RemoveFirstEvent(&event);
+ else
+ err = fRealTimeQueue.RemoveFirstEvent(&event);
+
+ if (err == B_OK) {
+ lateness -= TimeSource()->RealTime();
+ if (lateness < 0)
+ lateness = 0;
+ DispatchEvent(&event, lateness, hasRealtime);
+ }
+ } else if (err != B_OK)
+ return;

Best regards,
-Stephan


Other related posts: