[AVIFIL32] question about ACMStream_fn[AddRef|Release] in acmstream.c

[Paul Vriens]

Hi,

during my code cleanup I came to acmstream.c and found the following:

static ULONG WINAPI ACMStream_fnAddRef(IAVIStream *iface)
{
  IAVIStreamImpl *This = (IAVIStreamImpl *)iface;

TRACE("(%p) -> %ld\n", iface, This->ref + 1);

/* also add reference to the nested stream */
  if (This->pStream != NULL)
    IAVIStream_AddRef(This->pStream);

return ++(This->ref);
}

the AddRef can be cleaned up.

the Release however looks buggy:

static ULONG WINAPI ACMStream_fnRelease(IAVIStream* iface)
{
  IAVIStreamImpl *This = (IAVIStreamImpl *)iface;

TRACE("(%p) -> %ld\n", iface, This->ref - 1);

if (This->ref == 0) {
    /* destruct */

This means I have to do a Release twice before it actually goes into the
if statement, because the decrementing takes place at the end of the
Release function.

Can anybody confirm my finding.

Cheers,

Paul.

[AVIFIL32] question about ACMStream_fn[AddRef|Release] in acmstream.c

Paul Vriens  writes:
> Hi,
>
> during my code cleanup I came to acmstream.c and found the following:
>
> static ULONG WINAPI ACMStream_fnAddRef(IAVIStream *iface)
> {
>   IAVIStreamImpl *This = (IAVIStreamImpl *)iface;
>
> TRACE("(%p) -> %ld\n", iface, This->ref + 1);
>
> /* also add reference to the nested stream */
>   if (This->pStream != NULL)
>     IAVIStream_AddRef(This->pStream);
>
> return ++(This->ref);
> }
>
> the AddRef can be cleaned up.

I don't really know what you want to cleanup here, looks very clean to me.
> the Release however looks buggy:
>
> static ULONG WINAPI ACMStream_fnRelease(IAVIStream* iface)
> {
>   IAVIStreamImpl *This = (IAVIStreamImpl *)iface;
>
> TRACE("(%p) -> %ld\n", iface, This->ref - 1);
>
> if (This->ref == 0) {
>     /* destruct */
>
> This means I have to do a Release twice before it actually goes into the
> if statement, because the decrementing takes place at the end of the
> Release function.
>
> Can anybody confirm my finding.

You are right, the release method is buggy. the comparision must check
against one instead of zero or the decrement in the return statement
must be moved before this if-statement.

Michael Günnewig

[AVIFIL32] question about ACMStream_fn[AddRef|Release] in acmstream.c

[Paul Vriens]

Hi Michael,
> I don't really know what you want to cleanup here, looks very clean to me.

The cleanup I'm currently working on is changing This->ref[++|--] and
friends into Interlocked * functions for thread safety (see Janitorial
page on WineHQ).
> You are right, the release method is buggy. the comparision must check
> against one instead of zero or the decrement in the return statement
> must be moved before this if-statement.
>
> Michael Günnewig

I will create a patch for it and send it to wine-devel first, so at least
you could have a look after that. There is another place where I found the
same construct (ICMStream_fnRelease in icmstream.c).

After the above is fixed, I will continue my AddRef/Release code cleanup
on this dll.

One question remains though, what do I do with:

/* also release reference to the nested stream */
  if (This->pStream != NULL)
    IAVIStream_Release(This->pStream);

should this be done regardless of the value of This->ref ?

Cheers,

Paul.

[AVIFIL32] question about ACMStream_fn[AddRef|Release] in acmstream.c

[Paul Vriens]

> One question remains though, what do I do with:
>
> /* also release reference to the nested stream */
>   if (This->pStream != NULL)
>     IAVIStream_Release(This->pStream);
>
> should this be done regardless of the value of This->ref ?

Hi Michael,

forget the question. I just saw that the above is also in the if-loop.
Will send a patch this evening and right after that a This->ref cleanup
for avifil32.

Cheers,

Paul.