Cool VL Viewer forum

View unanswered posts | View active topics It is currently 2024-03-28 11:12:21



Reply to topic  [ 46 posts ]  Go to page Previous  1, 2, 3, 4, 5  Next
IO Completion Port based LLLFSThread for windows & Linux 
Author Message

Joined: 2009-03-17 18:42:51
Posts: 5523
Reply with quote
kathrine wrote:
the mOffset stuff is kind of weird for READ...

The old code does an llassert(mOffsett >= 0) and then goes on to handle the case that mOffset <0 for seeking from the end. So that should basically never happen.
Not really, because llassert() is only actually executed for DEBUG builds... :-P

But yes, that llassert() shall be removed... or replaced with llassert_always() and the mOffset < 0 stuff removed.


2021-09-30 21:39:13
Profile WWW

Joined: 2011-10-07 10:39:20
Posts: 181
Reply with quote
I think i found the sound issue.

The problem is the fetching of the completions, it seems that for sounds it never fires.

If i move the Completion part into a LLLFSThread::runCondition() method instead of the processNextRequest() it fires and sounds start to play again.

Might be related how the audio part handles its completions.


2021-09-30 22:26:13
Profile

Joined: 2009-03-17 18:42:51
Posts: 5523
Reply with quote
kathrine wrote:
I think i found the sound issue.

The problem is the fetching of the completions, it seems that for sounds it never fires.

If i move the Completion part into a LLLFSThread::runCondition() method instead of the processNextRequest() it fires and sounds start to play again.

Might be related how the audio part handles its completions.

Just a quick update with some more (superficial) testing I made: sound does not seem to be the only affected type of assets. I also get a lot of animation duplicate requests warnings in the log...


2021-09-30 22:41:34
Profile WWW

Joined: 2011-10-07 10:39:20
Posts: 181
Reply with quote
For sounds it is the following mix of problems (for Windows currently, but probably Linux is the same).

1) mBytesRead is reset to 0 in ReadFile/WriteFile before handing off the OVERLAPPED Calls
2) The llaudiodecodemgr uses mBytesRead as a marker for completion, as it sets it to -1 first and then sets it to the correct value in completed().
3) So when the async I/IO starts, the file handle is in use, but then the audio part checks for size=0, logs an error and tries to fix it by deleting the file (which fails).

That spams the log file and can be fixed by using NULL for the result param, so it does not get zeroed.
DWORD status = ReadFile(mFile, mBuffer, mBytes, NULL, &mOverlapped);

The second (and main) issue seems to be that the GetQueuedCompletionStatus(mIOCPPort, &bytesProcessed, &key, &result, 0); or equivalent io_uring_peek_cqe() call isn't reached, as the LFS thread goes to sleep. If i move the completions to the runCondition() method instead, the completions get processed just fine.


2021-09-30 23:04:10
Profile

Joined: 2011-10-07 10:39:20
Posts: 181
Reply with quote
I did a few changes on the Windows side of the patch.

This restores sound for me and i noticed no strange log messages anymore with it present. Changes are mostly the same as describe, i moved the Completion into the "threadedUpdate" method of the QueuedThread, not sure if thats a good place or if it should be moved to somewhere else alltogether.
With some more fixes, it works with proper sound on Linux as well, at least for my quick tests.

I didn't see the other messages you mentioned about animations or other assets, hopefully those are also now working.


Attachments:
io_uring_v5.patch.gz [19.83 KiB]
Downloaded 100 times
2021-10-01 16:23:46
Profile

Joined: 2009-03-17 18:42:51
Posts: 5523
Reply with quote
kathrine wrote:
I did a few changes on the Windows side of the patch.

This restores sound for me and i noticed no strange log messages anymore with it present. Changes are mostly the same as describe, i moved the Completion into the "threadedUpdate" method of the QueuedThread, not sure if thats a good place or if it should be moved to somewhere else alltogether.
With some more fixes, it works with proper sound on Linux as well, at least for my quick tests.

A couple of things seem off to me in this new patch:
  • There's obviously a missing "sRingInitialized = true;" for Windows, just after the "mIOCPPort = CreateIoCompletionPort(...);" line.
  • You changed mBytesRead(0) to mBytesRead(-1) in the request constructor but this might break the non-io_uring code (used when sUseIoUring is false). I suggest instead using mBytesRead(sUseIoUring ? -1 : 0)

Also, the LLLFSThread::processNextRequest() method can be removed entirely and the matching processNextRequest() method in LLQueueThread can be made non-virtual again.


2021-10-01 19:42:17
Profile WWW

Joined: 2009-03-17 18:42:51
Posts: 5523
Reply with quote
Well, I have been using this slightly modified patch (with the few changes described above), but it still fails to play (non-cached) sounds for me under Linux (not yet tested under Windows here):


Attachments:
io_uring-v6.patch.gz [8.33 KiB]
Downloaded 95 times
2021-10-01 20:22:07
Profile WWW

Joined: 2011-10-07 10:39:20
Posts: 181
Reply with quote
I looked at your updated patch.

The issue with sound comes from setting mBytesRead = 0 too early (while the async read/write is still in flight).

So adding the "mBytesRead = mBytes; // Success" back, breaks sound again.

The problem/reason for this is in llaudio/llaudiodecodemgr.cpp around line 495:
Code:
      mBytesRead = -1;
      mFileHandle = LLLFSThread::sLocal->write(mOutFilename,
                                     &mWAVBuffer[0], 0,
                                     mWAVBuffer.size(),
                                     new WriteResponder(this));
   }

   if (mFileHandle != LLLFSThread::nullHandle())
   {
      if (mBytesRead < 0)
      {
         return false; // not done
      }
      if (mBytesRead == 0)
      {
         llwarns_once << "Unable to write file for " << getUUID() << llendl;
         mValid = false;
         return true; // we've finished
      }
   }

   mDone = true;

#if 0
   LL_DEBUGS("Audio") << "Finished decode for " << getUUID() << LL_ENDL;
#endif

   return true;


As you can see, it uses the -1 as a marker for "not yet done" and 0 or more as marker for "done". So if the lllfsthread codes sets the mBytesRead to anything but -1 before the actual async read/write is completed (for a sound), this breaks down and the error handler for sounds kicks in and loading of sounds fails.

That should work for my patch and i see .dsf files created in the cache area properly. I wonder why it doesn't work for you.

Do the completions get collected ( does io_uring_peek_cqe return 0 and populate a cqe? Your patch does "have_work = io_uring_peek_cqe()" which has the logic reversed ).
I had some issues when the LFS thread was set as idle and slept and completions would not get collected in my first tries.

Edit: Missed the (non-cached) part. Will have to check it works properly with a cleaned cache.


2021-10-01 20:49:02
Profile

Joined: 2016-06-19 21:33:37
Posts: 337
Location: Columbus area, OH, USA
Reply with quote
I grabbed Henri's patch and applied it. A quick build and test has sounds playing for me in Linux. Is there anything I can do to help test or narrow down? The only 'custom' I have in my builds is using the new curl.


2021-10-01 20:52:58
Profile

Joined: 2009-03-17 18:42:51
Posts: 5523
Reply with quote
kathrine wrote:
So adding the "mBytesRead = mBytes; // Success" back, breaks sound again.
I removed that from my latest patch... Might have uploaded an intermediate "v6" patch... Still no joy with that line removed anyway.

Quote:
That should work for my patch and i see .dsf files created in the cache area properly. I wonder why it doesn't work for you.
It does not work for me...

Quote:
Do the completions get collected ( does io_uring_peek_cqe return 0 and populate a cqe? Your patch does "have_work = io_uring_peek_cqe()" which has the logic reversed ).
Again, this was since fixed...
Quote:
I had some issues when the LFS thread was set as idle and slept and completions would not get collected in my first tries.
I can only play *cached* (in the asset cache) sounds here...


2021-10-01 20:53:57
Profile WWW
Display posts from previous:  Sort by  
Reply to topic   [ 46 posts ]  Go to page Previous  1, 2, 3, 4, 5  Next

Who is online

Users browsing this forum: No registered users and 17 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group
Designed by ST Software.