[FFmpeg-devel] [PATCH 1/2] Audio Video Filtering using threads & semaphores

[Nicolas George]

Le nonidi 9 thermidor, an CCXX, Manjunath Siddaiah a écrit :
> 1. As far as I understand, you intend to run the whole filtering process
> in a different thread from the ffmpeg command-line tool. Is that right?

> Answer: yes

Ok. This is simpler than anything else.

> 2.Some comments about what the variables mean, and their relationships
> (who is locking what) would be useful.
> Answer:Will add the comments to the variables appropriately.

Thanks.


> 3. It looks like this function is used as argument to pthread_create. The
> prototype is wrong, and gratuitously so since the return value is
> meaningless. Did your compiler not produce a warning for that?

> Answer:I will confirm about that, if there is a warning I will change the
> proto-type.

Also thanks.

> 4. ost->avfilter_thread_alive is shared: theoretically it must be protected by a mutex.

> Answer:   Yes theoretically should be protected. But Ffmpeg thread will
> use this variable only at the start of transcode and at the end of
> transcode. Rest of the time it is fully owned by the filter thread.

It is changed by the main thread and read by the filter thread. Without
locks, the compiler would be authorized to optimize the change away if it is
able to detect it does nothing, or the processor to keep the change in cache
and not propagate to the other processors. Mutexes contain enough compiler
and processors magic to force the synchronization.

Of course, this is completely theoretic here, because the code is much too
complex for that to happen.

> 5.  It looks wrong: you are doing the expensive stuff
> (avfilter_graph_request_oldest is what triggers most of the work) while
> holding the lock, and you release the lock just before you alter the
> variable     with the same name as the lock.

> Answer: one wrong thing here is, alter the variable after mutex unlocking.
> I will change this.  Yes avfilter_graph_request_oldest is the expensive
> stuff, that's why it is put on thread to get the filtered frame. 

Then there is definitely something wrong there: there is a general principle
with threads, at least as implemented by pthreads: never do long or
expensive operations while holding a lock. And the reason is quite simple:
if you do so, you risk blocking another thread that needs the lock too and
losing all benefits of parallelism.

> 6. Rewriting the whole body of poll_filters() seems wrong: one of the
> versions will unavoidably bitrot, i.e. not get the bugfixes that the other
> version will receive.

> Answer: I can rewrite my changes in a single function. While developing
> the algorithm, I retained the original stuff to understand the code.

I can understand that. Unfortunately, by doing that you lose the benefit of
the revision control system: instead of merging the upstream changes with
your modified code, it will merge them with the commented out code and leave
your modified code to the old version.

> As far as I know we have distinguishable filter-graph for each output
> stream.

I think you missed the -filter_complex feature: filters graphs can now have
several inputs and several outputs. You definitely need to update your code
to have one thread per graph and not one thread per stream.

> Answer: with the above responses, revisit and let me know your further
> comments.

Thanks for the clarifications. There are a few minor points that definitely
need fixing, but on the whole, I still think you could achieve your goal
with much less changes. On the whole, apart from creating and reaping the
threads, alterations around buffersrc_add, buffersink_get and request_oldest
should be enough, except if I am missing something.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120728/d3d1c51f/attachment.asc>