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

[Manjunath Siddaiah]

Hi Nicolas,

I am working on modifying my last submission to put the scalar on threading for multistep transcoding. 

I have gone through latest ffmpeg code to understand the flow.
If AV_BUFFERSRC_FLAG_PUSH is enabled while calling " av_buffersrc_add_ref", which in turn call scaling function " swScale" .
If AV_BUFFERSRC_FLAG_PUSH is not enabled while calling " av_buffersrc_add_ref" scaling function " swScale" is called from "avfilter_graph_request_oldest".

Why we have these different modes of operation ? 
And please suggest where I should do the work for threading scaling function for multistep transcoding, so that changes should be good for submission.

Thanks & Regards
Manjunath 

-----Original Message-----
From: ffmpeg-devel-bounces at ffmpeg.org [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of Nicolas George
Sent: Saturday, July 28, 2012 9:38 AM
To: FFmpeg development discussions and patches
Subject: Re: [FFmpeg-devel] [PATCH 1/2] Audio Video Filtering using threads & semaphores

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