[FFmpeg-devel] [PATCH v8 01/11] avformat: Add fifo pseudo-muxer

[Nicolas George]

Le decadi 30 thermidor, an CCXXIV, Jan Sebechlebsky a écrit :
> I am not sure if I understand this. Do you mean thread queue function which
> would set
> certain flag that the queue should be flushed and flushed the queue at
> certain point in time (next receive call?)?

That is a bit too specific but you have the gist of it.

As a rule, the more communication mechanisms are mixed together, the harder
it becomes to get things right and efficient. Furthermore, using low-level
functions is often more tricky than high-level functions. In this code, you
are doing both: mixing a message queue with a low-level flag and mutex. It
would be more efficient if the message queue could take care of all your
needs with a simple API.

In that particular case, we can copy what other protocols do, because they
certainly gave it enough thought: allow a single out-of-band message that
jumps directly in the front of the queue. Then you just need to make that
message need "queue needs flushing" to achieve your goal.

Adding the out-of-band message could be done with a flag,
AV_THREAD_MESSAGE_OOB or AV_THREAD_MESSAGE_IMMEDIATE.

The tricky part is to make sure that it can not fail or block. With regard
to normal in-band messages, it can be done by having an extra, separate,
slot for the out-of-band message. But it still blocks if a second
out-of-band is added before the first one is consumed. I think the best
solution for that is to require a combine callback function:

int combine_messages(void *cur_v, const void *new_v, void *opaque)
{
    MessageType *cur = cur_v, const *new = new_v;

    cur->flags |= new->flags;
    cur->count += new->count;
    //cur->first_ts = cur->first_ts;
    cur->last_ts = new->last_ts;
    return 0;
}

If the combine callback can not fail, then the whole process can not fail
either, and we have won.

But see below.

> The current packet is discarded here. We discussed with Marton how to drop
> packets and agreed that flushing the whole queue should be OK - it is done
> within single critical section in av_thread_queue_flush(), flushing just
> single packet might not be enough in some situations. The reason why is it
> done in worker thread and not in this call is that flushing the queue can
> get quite costly -> dereferencing larger number of packets can lead to many
> free() calls which may be costly, the idea was to move potentially costly
> operation to worker thread.
>     It may be a good thing to make this configurable (allow user to set how
> many messages to flush on overflow) and add function to thread queue which
> would flush certain number of messages in single critical section.

I am really not convinced by these arguments, especially with the code
complexity their conclusion causes.

First of all: "might in some situations": as you point in the next
paragraph, it needs to be configurable. Eventually, the feature can go in
without. But the design should make configuration easy. In this particular
case, it does not, because it structurally confuses the size of the queue
and the number of messages to discard to smooth an overrun.

Furthermore, there is somewhat of a contradiction in the arguments between
the requirement of a single critical section and the worry about the cost of
the free()s. If the free()s are expensive, then they should not happen in a
critical section.

As it happens, I think my proposal takes care of all four points in a single
sweep. I could say, wondering if someone will catch the reference, that my
plan is sheer elegance in its simplicity:

In the thread message queue, focus on the actual muxing and the recovering.
That is already a full task. Leave the handling of overruns to the main
thread -> takes care of the code complexity.

In the main thread, discard the packets one at a time, as they arrive -> a
single free() per packet, which is considered normal handling cost in a
muxer -> takes care of the cost of free()s.

Still in the main thread, set a field "discard_n_next_packets" -> takes care
of discarding several consecutive packets to reduce artifacts (which may not
always for the best; I think speech audio codecs are better with sparse
missing packets, for example). Logically, the "discard_until_keyframe"
belongs at the same place; and the logic could also include some kind of
"discard_until_less_than_n_packets_in_queue".

And last of the four: nothing above requires any critical section.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160818/5bdcbad7/attachment.sig>