On Thu, 13 Jun 2019, Andreas Håkon wrote:
Hi Andriy,
I'm glad you're interested in this patch.
> This patch implements a new optional "parallel muxing mode" in the MPEGTS
muxer.
> The strategy that implements the current mux (selected by default) is based on
> writing full PES packages sequentially. This mode can be problematic when
using
> with DTV broadcasts, as some large video PES packets can delay the writing of
> other elementary streams.
Could you go into more detail as to why this causes problems for DTV broadcasts?
Because two reasons:
1) When substreams of multiple services are interlaced, broadcast errors are
minimized.
Burst errors in the signal only interfere with a small number (or just one)
packet
of each program, so it's possible that the error is camouflaged.
In contrast, when using CONSECUTIVE pid packets, burst errors are more
significant.
2) Due to buffer restrictions on DTV broadcasts. If you include more than one
video
stream with large PES packets, sequential writing can cause buffer problems at
reception because other PES packets arrive a little later.
Ok. But I believe the correct term for this is "interleaved". I have
never heard "interlaced" used for TS packets.
> +#define PES_START 1 /* 0000 0001 /
> +#define PES_FULL 2 / 0000 0010 */
> +#define UNUSED_FLAG_3 4 /* 0000 0100 /
> +#define UNUSED_FLAG_4 8 / 0000 1000 /
> +#define UNUSED_FLAG_5 16 / 0001 0000 /
> +#define UNUSED_FLAG_6 32 / 0010 0000 /
> +#define UNUSED_FLAG_7 64 / 0100 0000 */
Is it relevant to include these?
No. It's not necessary. However, they make the code more compressible and can be
used in the future for other flags.
Don't include them if they are not needed. It would be also a good idea
to name them xxx_FLAG if these are indeed flags. e.g.:
PES_START_FLAG
PES_FULL_FLAG
PES_NEEDS_END_FLAG
also I see no reason that PES_NEEDS_END is 128. Just make it 4. And keep
the indentation pretty.
> - ts_st->payload = av_mallocz(ts->pes_payload_size);
> + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD :
ts->pes_payload_size);
Could you clarify why this needs to be changed?
Sure! Because when you write in parallel you're delaying the writing of the PES
packet.
So you need to save the entire PES packet. And the ts->pes_payload_size it's
defined to
a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file:
#define DEFAULT_PES_HEADER_FREQ 16
#define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170)
OK, but you shold override ts->pes_payload_size, and not the malloc, no?
There is code in mpegts_init which sets pes_payload_size, you should force
it to MAX_PES_PAYLOAD there before doing the round up.
> + for (i = 0; i < ts->nb_services; i++) {
> + service = ts->services[i];
> ...
> }
Why do you need the loop over the services here? It seems unrelated unless
I missed something.
I explained it in my original message...
The current code has a bug and it doesn't set correctly the value of the
service->pcr_pid. And this patch requires correct values of pcr_pid for each
program. Please note that this patch makes complete sense when mixing multiple
video streams, such as when using multiple programs.
This should be a separate patch then. I suggest implementing this before
adding interleaving support.
> - - NOTE: 'payload' contains a complete PES payload.
> - - NOTE2: When 'mode' < -1 it writes the rest of the payload (without
header);
> - - When 'mode' = -1 it writes the entire payload with the header;
> - - when 'mode' = 0 it writes only one TS packet with the header;
> - - when 'mode' > 0 it writes only one TS packet.
>
Enum for mode would make more sense to me
Yes and not. The code is more compact in this case with numerical values, as you
don't need to do checks when you call to the function.
The code might be more compact, and tons of less readable. Use enums, or
defines. You might also use flags for mode, if that makes the code more
readable.
> + if ((ts_st->pes_flags & PES_NEEDS_END) && payload_size == len) {
This seems unrelated to your commit..
If it is, you can remove PES_NEEDS_END
Sorry, no! That's completely related to this patch. Let me explain:
Writing Telext PES packets it's an special case. The function writes at end 1
byte. The flag PES_NEEDS_END is used in this case to indicate this case. Please
note that the function with this patch is called multiple times for the same PES
packet. And only when writing the header at the start this check can be
determined.
So we need to save this information. If not, some packets are malformed.
> + payload_size = 0; // Write one packet only then exit
Why not just break?
Sorry? That's the last line of the loop! What's the advantage of breaking the
loop here?
Because you break out of it later anyway since the loop condition is
"payload_size > 0".
Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
