[FFmpeg-devel] [PATCH 2/2] dshow: handle events in graph

[Don Moir]

> Hi,
>
> On Wed, Dec 5, 2012 at 11:54 PM, Don Moir <donmoir at comcast.net> wrote:
>>> From: "Ramiro Polla" <ramiro.polla at gmail.com>
>>> To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
>>> Sent: Tuesday, December 04, 2012 1:23 AM
>>> Subject: [FFmpeg-devel] [PATCH 2/2] dshow: handle events in graph
>>>> Thanks to Don Moir for pointing out the problem and providing a fix.
>>>>
>>>> Don, could you please test that this patch fixes the problem for you?
>>>>
>>>> Ramiro
>>>>
>>> Ramiro, your patch sort of works but had a couple bugs.
>>>
>>> o - fixes return value in dshow_read_header. Your patch for this had a
>>> couple misplaced ')' in it.
>
> Fixed in the other patch.
>
>>> o - re-added ctx->eof. After the device has failed, this is a check to
>>> stop trying to read any more packets in dshow_read_packet.
>>> Without this check and someone calls av_read_frame after it has already
>>> failed will result in a hang.
>
> Good. I would imagine one shouldn't call av_read_frame() after it
> returned an error, but it's better to be safe here.

Yeah, but for sure it will happen.

> In your code
> FFmpeg would loop endlessly if AVFMT_FLAG_NONBLOCK was set. I changed
> the way dshow_read_packet() works to take that into account. Please
> check to see if it's still correct.

Yeah missed that with EAGAIN return value. I was trying to avoid checking the events every loop and wait for the event to be 
signaled. I tested your code change for performance and it was fine. I think I prefer the following though because its a bit 
cleaner:

/**
 * Checks media events from DirectShow and returns non-zero on error or EOF.
 * Also purges all events that might be in the event queue to stop the trigger of
 * event notification.
 */
static int dshow_check_event_queue(struct dshow_ctx *ctx)
{
    LONG_PTR p1, p2;
    long code;
    while (IMediaEvent_GetEvent(ctx->media_event, &code, &p1, &p2, 0) != E_ABORT) {
        if (code == EC_COMPLETE || code == EC_DEVICE_LOST || code == EC_ERRORABORT)
            ctx->eof = 1;
        IMediaEvent_FreeEventParams(ctx->media_event, code, p1, p2);
    }
    return ctx->eof;
}

...
         if (!pktl) {
            if (dshow_check_event_queue(ctx))
                break;
            else if (s->flags & AVFMT_FLAG_NONBLOCK)
                 return AVERROR(EAGAIN);
            else
                 WaitForMultipleObjects(2, ctx->event, 0, INFINITE);
         }
...

Also appears some effort is being done to remove unneeded braces.

>>> o - dshow_check_event_queue did not free the event when eof was detected.
>>> I modified it to take the ctx struct instead of media
>>> event so that ctx->eof could be set.
>
> I made the function have a return code and set eof in dshow_read_packet().
>
>>> o - added CoUninitialize to dshow_read_close. Every call to CoInitialize
>>> should have a matching CoUnititalize. Changed the position
>>> of CoInitialize in dshow_read_header so we always know to call
>>> CoUnintialize without flagging it.
>
> Fixed this is a separate patch (along with the other hunk), thanks for
> spotting it.
>
>>> With event handling added to dshow.c and fix for ret value in
>>> dshow_read_header I can't get any abnormal behavior like hangs.
>>> Without the changes I can get to fail in at least 3 different ways.
>>>
>>> I will be working in the area of camera support for awhile and will
>>> continue to test. So if any kind of modifications to dshow
>>> support or device listing is wanted, now would be a good time for them.
>
> If you want to implement crossbar support that would be nice =). I no
> longer spend much time with FFmpeg-related stuff, so for me it'll be
> only these issues and maybe add device/option listing for dshow if it
> gets implemented.
>
> [...]
>
> Thanks for the review.
>
> Ramiro

Thanks Ramiro. I think Michael is preparing for device list support.