[Committed] POSIX Compliant "TaskSet" locking

[scsiguy]

Perhaps a year ago, there was a discussion on IRC about the locking in decomb.c/deinterlace.c and it relying on undefined/implementation specific behavior of POSIX mutexes. That is, the ability for one thread to unlock a mutex owned by another thread. This particular behavior is explicitly disallowed by the FreeBSD pthreads implementation (it will terminate the program). To avoid this, I used POSIX condition variables to control the ganged computation in HandBrake. The original set of patches, which are included in the FreeBSD ports collection version of HandBrake, had too much code duplication for my tastes, so I never submitted them for inclusion in HandBrake. I've now re-factored the code into something I hope will be acceptable. My patches can be found here:

http://pastebin.ca/1817937

The code adds the concept of a taskset: a group of cooperating threads, working on the same task. The taskset API encapsulates all of the tedium of managing the threads and synchronization primitives. A consumer of taskset, need only create, cycle for each unit or work, and destroy the taskset in order to complete the task. The threads of the task are provided with two simple functions to wait for work and to signal work completion. The use of the API allows for a ~200 line reduction in decomb.c alone.

When this was last discussed, concerns were raised about performance. Since the original implementation does not run on FreeBSD, I do not have before and after comparisons. I have access to Linux VMs on a Xen system, and I can run before and after tests there, but I fear that the introduction of virtualization could make the results unreliable. My hope is that some of the developers here will try the patch an report their results.

I believe that the abstraction offered by the taskset concept has value on it's own. If the performance of my implementation isn't found to be as good or better than the locking from before, I will implement a version that uses the old locking scheme. A given port/variant of HandBrake could then select the appropriate taskset backend for it. This would support my eventual goal of adding and supporting a FreeBSD port of HandBrake from within the HandBrake SVN tree.

[JohnAStebbins]

After I fixed an initialization problem in decomb.c, this patch worked wonderfully.
http://handbrake.fr/pastebin/pastebin.php?show=1284

You dropped an initialization of pv->yadif_arguments.dst = NULL

It improves speed signficantly. On linux using the equivalent to x264's ultrafast preset:

Code: Select all

filter        fps orig     fps taskset
-----------   ----------   ----------
decomb        140          177 
deint slow    115          258 
deint slower  98           240 

For reference, ultrafast with no filters is 400 fps

[scsiguy]

It looks like (based on a search through your diffs) that I made a similar mistake in rotate.c. I won't be in a place to submit a new set of diffs for a few hours, so if you don't mind publishing a fix for this too, please do.

Did you visually compare the decomb/deinterlace results of the two runs? I just want to double check that the performance jump is not a side effect of some bug I've introduced.

Thanks for testing out the patch!

[JohnAStebbins]

This fixes the rotate initialization.
http://handbrake.fr/pastebin/pastebin.php?show=1286

But decomb and deinterlace are not working right. Looks as if slices from multiple frames are being rendered to output frames.

[scsiguy]

One problem is that pthread_cond_wait() can suffer from spurious wakeups on some platforms. See http://www.opengroup.org/onlinepubs/007 ... _wait.html for details. FreeBSD doesn't seem to do this, but obviously Linux does. Can you try this updated patch?

http://pastebin.ca/1852546

[JohnAStebbins]

Checking the condition again after cond_wait is the right thing to do, but in this case, that wasn't the problem. It's a problem in bit_nclear. The C standard says it is undefined to shift by negative values or values >= wordsize. You we shifting by wordsize in some cases. Here's the fix http://handbrake.fr/pastebin/pastebin.php?show=1292

It now generates correct output for me and the performance looks to be the same as before.

[scsiguy]

I fixed another instance of the "shift by word size or larger" and have regenerated the patch set. Nice catch!

http://pastebin.ca/1853216

--
Justin

[JohnAStebbins]

Um, where is the other one. I don't see it.

And the change you made to how I was doing it is good, but you reverted an occurrence of the problem that I had fixed

Code: Select all

+                             |  ( 0xFFFFFFFF << ( ( stop_pos & 0x1F ) + 1 ) ) );

(stop_pos & 0x1f) + 1 can be 32.

[scsiguy]

The one I thought you missed, you properly fixed in the previous patch, I just missed it when parsing the full file diff. I also missed your fixes for the left shift operators. Here's another attempt, again using the '0 bit in mask' trick to avoid the extra shift operation.

http://pastebin.ca/1853308

Just to make it easier to see, this is what bit_nclear() now looks like.

Code: Select all

static inline void
bit_nclear(uint32_t *bit_map, int start_pos, int stop_pos)
{
    int start_word = start_pos >> 5;
    int stop_word  = stop_pos >> 5;

    if ( start_word == stop_word )
    {

        bit_map[start_word] &= ( ( 0x7FFFFFFF >> ( 31 - (start_pos & 0x1F ) ) )
                             |  ( 0xFFFFFFFE << ( stop_pos & 0x1F ) ) );
    }
    else
    {
        bit_map[start_word] &= ( 0x7FFFFFFF >> ( 31 - ( start_pos & 0x1F ) ) );
        while (++start_word < stop_word)
            bit_map[start_word] = 0;
        bit_map[stop_word]  &= 0xFFFFFFFE << ( stop_pos & 0x1F );
    }
}

[Hilbe]

Any update on the FreeBSD port?

I'd love some dummy friendly compilation instructions to get Handbrake compiled on FreeBSD/AMD64.

[jbrjake]

This stuff is overdue for being committed, I just haven't had an opportunity yet to test the code and integrate it with filter changes that have occurred in the intervening time. I really am sorry scisguy, and it really is at the top of my todo list.

[Hilbe]

*Bump*

Any update on getting this into the FreeBSD port?

[JohnAStebbins]

We haven't forgotten about it.

[jbrjake]

[gt46l]

Has there been any progress on this? Nix is good BSD is better. Sorry for the bump.

[s55]

Not quite yet. Not many folk have much time to work on the project these days, so things are slow going. Hopefully won't be too long now that the release isn't blocking things.

[s55]

Pushing this thread up to the top of the priority list given it's been about for a while and not submitted yet.

http://handbrake.fr/pastebin/pastebin.php?show=1292 still applies. The patches you put on pastebin.ca seem to have disappeared (pastebin.ca seems to be down) so the above seems to be the latest.

Started doing some new encode tests with this applied to the trunk. No problems so far. Will post results later.

[Deleted User 11865]

The one I thought you missed, you properly fixed in the previous patch, I just missed it when parsing the full file diff. I also missed your fixes for the left shift operators. Here's another attempt, again using the '0 bit in mask' trick to avoid the extra shift operation.

http://pastebin.ca/1853308

Just to make it easier to see, this is what bit_nclear() now looks like.

Code: Select all

static inline void
bit_nclear(uint32_t *bit_map, int start_pos, int stop_pos)
{
    int start_word = start_pos >> 5;
    int stop_word  = stop_pos >> 5;

    if ( start_word == stop_word )
    {

        bit_map[start_word] &= ( ( 0x7FFFFFFF >> ( 31 - (start_pos & 0x1F ) ) )
                             |  ( 0xFFFFFFFE << ( stop_pos & 0x1F ) ) );
    }
    else
    {
        bit_map[start_word] &= ( 0x7FFFFFFF >> ( 31 - ( start_pos & 0x1F ) ) );
        while (++start_word < stop_word)
            bit_map[start_word] = 0;
        bit_map[stop_word]  &= 0xFFFFFFFE << ( stop_pos & 0x1F );
    }
}

I merged this with pastebin 1292: http://paste.handbrake.fr/pastebin.php?show=2163

That way we should have an up-to-date (I hope) version for reference.

[JohnAStebbins]

For people interested in this, the patch is being tracked and kept up to date here:
https://reviews.handbrake.fr/r/185/

[JohnAStebbins]

After more than 2 years, finally committed.
https://trac.handbrake.fr/changeset/4685

Could have been worse. Bob took 3 years