On Sun, Dec 06, 2015 at 12:38:09PM +0100, Nicolas George wrote: [...] > > +/* same as worker_data but shuffled for testing purpose */ > > Is it really useful? If you merge both, you can probably get rid of the > macros below with just a conditional for the function on pthread_create(). >
I actually want to make sure there is a distinct difference between the 2
data, because in practice that's what happen with the senders and
receivers.
> > +struct reader_data {
>
> Nit: worker/reader sounds like a mixed metaphor.
>
Renamed to senders and receivers.
[...]
> > + av_log(NULL, AV_LOG_INFO, "worker #%d: workload=%d\n", wd->id,
> > wd->workload);
> > + for (i = 0; i < wd->workload; i++) {
>
> > + if (rand() % wd->workload < wd->workload / 10) {
>
> Nit: using module for PRNG is not recommended, it gives very bad results
> with LCG.
>
I don't think it really matters in this case, but feel free to change.
> > + av_log(NULL, AV_LOG_INFO, "worker #%d: flushing the queue\n",
> > wd->id);
> > + av_thread_message_flush(wd->queue);
> > + } else {
>
> Nit: inconsistent structure: if/else here, if+continue for readers.
>
fixed
[...]
> > + if (ac != 8) {
> > + av_log(NULL, AV_LOG_ERROR, "%s <max_queue_size> "
> > + "<nb_workers> <worker_min_send> <worker_max_send> "
> > + "<nb_readers> <reader_min_recv> <reader_max_recv>\n",
> > av[0]);
> > + return 1;
> > + }
>
> Nit: huge number of arguments is annoying. Maybe give sensible default
> values and use getopt()?
>
A bit too much pain for me to update it right now. Since it's not
blocking, I'll push as is. Feel free to change it.
[...]
> > + ret = av_thread_message_queue_set_free_func(queue, free_frame);
> > + if (ret < 0)
> > + goto end;
>
> Returns void now, or did I review the wrong version of the patch?
>
Yeah I noticed later and fixed it locally.
[...]
> > +end:
> > + av_thread_message_queue_free(&queue);
> > + av_freep(&workers);
> > + av_freep(&readers);
>
> Ideally, at this point the driving thread should have the readers and
> writers compare notes to see that no message was lost or handled
> twice. But that is annoying.
>
I think valgrind will handle most of it just fine. So FATE should cover it
properly.
[...]
Patch applied, thanks
--
Clément B.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
