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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel