[FFmpeg-devel] [PATCH 2/3] fate/api: test threadmessage

Clément Bœsch u at pkh.me
Mon Dec 7 11:42:58 CET 2015


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151207/4d48b25e/attachment.sig>


More information about the ffmpeg-devel mailing list