[FFmpeg-devel] New patch for mpegts.c
JULIAN GARDNER
joolzg at btinternet.com
Wed Oct 17 18:51:28 CEST 2012
----- Original Message -----
> From: Michael Niedermayer <michaelni at gmx.at>
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc:
> Sent: Wednesday, 17 October 2012, 17:36
> Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>
> On Wed, Oct 17, 2012 at 08:46:07AM +0100, JULIAN GARDNER wrote:
>>
>>
>>
>>
>>
>>
>> ----- Original Message -----
>> > From: Michael Niedermayer <michaelni at gmx.at>
>> > To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
>> > Cc:
>> > Sent: Wednesday, 17 October 2012, 1:41
>> > Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>> >
>> > Hi
>> >
>> > On Tue, Oct 16, 2012 at 08:34:27PM +0100, JULIAN GARDNER wrote:
>> >>
>> >> >________________________________
>> >> > From: Michael Niedermayer <michaelni at gmx.at>
>> >> >To: FFmpeg development discussions and patches
>> > <ffmpeg-devel at ffmpeg.org>
>> >> >Sent: Tuesday, 16 October 2012, 17:16
>> >> >Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>> >> >
>> >> >On Tue, Oct 16, 2012 at 02:33:01PM +0100, JULIAN GARDNER
> wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >> ----- Original Message -----
>> >> >> > From: Michael Niedermayer <michaelni at gmx.at>
>> >> >> > To: FFmpeg development discussions and patches
>> > <ffmpeg-devel at ffmpeg.org>
>> >> >> > Cc:
>> >> >> > Sent: Tuesday, 16 October 2012, 15:05
>> >> >> > Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>> >> >> >
>> >> >> > On Tue, Oct 16, 2012 at 01:31:35PM +0100, JULIAN
> GARDNER
>> > wrote:
>> >> >> >>
>> >> >> >>
>> >> >> >> Whilst working on some new additions to MPEGTS
> support i
>> > found that the
>> >> >> > current code did not take into account the
> "Table
>> > Version Number",
>> >> >> > which caused FFMPEG to process every table that was
> in the
>> > TS. After adding the
>> >> >> > new code and validating against a 3 minute TS the
> counts on
>> > tables processed
>> >> >> > went down from 1360 to 3.
>> >> >> >
>> >> >> > can you provide a testcase that shows the
> improvment?
>> >> >>
>> >> >> Remove the check against the version number and run the
> code with
>> > debug and you will see hundreds of messags saying it has processed the
> table,
>> > but with the same version number, put the check back in and rerun and
> you will 3
>> > messages, well that is it the version number does not change
>> >> >
>> >> >well, this way we would test different things having a
> different TS
>> >> >stream as input and this may lead to different conclusions.
>> >> >
>> >> >The debug messages can also be suppresed with quite a bit
> less code
>> >> >than this patch. So being more verbose my question really is
> about
>> >> >a testcase that shows some improvment beyond commenting a
> debug
>> >> >message out, like showing a speed gain.
>> >> >
>> >> No it wont, this code will stop FFMPEG processing EVERY section
> that is
>> > sent to it, it will only process 1 PAT/PMT/SDT per table version, and
> when a
>> > table version changes this will also be processed.
>> >>
>> >> My Test streams, which i use/used for DVB Subtitles
>> >>
>> >> Name, Processed, Ignored, %
>> >>
>> >> BBC_DVBS2.ts, 24, 27643, .00009
>> >>
>> >> bbc.ts, 3, 581, 0.0005
>> >>
>> >> screenDVBx1, 2, 1652, 0.0001
>> >>
>> >> screenHD60sec, 2, 1622, 0.0001
>> >>
>> >> Wild.ts, 2, 431, 0.004
>> >>
>> >> 2 hour recording, 3, 71180, 0.000004
>> >>
>> >> I am not trying for a speed gain per se, I am trying to make the
> TS decoder
>> > work as per the spec, which it does NOT. this patch makes the
> groundwork for
>> > adhering to the spec that one step closer and by not processing
> 99.99998% of
>> > tables which are the same as the last 99.99998% there will be a
> speedup.
>> >
>> > If its about spec compliance, please quote the spec that requires
>> > this behavior.
>> >
>> ETS 300468 5.1.1 d
> http://www.etsi.org/deliver/etsi_en/300400_300499/300468/01.11.01_60/en_300468v011101p.pdf
>
> doesnt say one should discard things with equal version numbers
>
"When the characteristics of the TS described in the SI given in the present document change (e.g. new events start, different composition of elementary streams for a given service), then new SI data shall be sent containing the updated information. A new version of the SI data is signalled by sending a sub_table with the same identifiers as the previous sub_table containing the relevant data, but with the next value of version_number."
"document change" " the next value of version_number", so to me this means that if the data is THE SAME then the version number will be the same, and i guess that the guys who wrote the spec did this so you DONT have to process the SAME data over and over again, what it was added for was hardware filtering, which if you have spent 15+ years writing code for sat receivers etc you will know this. Why would you want to process possibly millions of sections which contain the SAME data, to just produce the same tables?.
Now if you keep on banging on about the .00000001% of people who concatenate 2 ts streams and do not follow the spec and either make sure the SI data is the same, which means the version number the same, or make the version number follow by becoming the next version then we are at an impasse, because i think your wrong on this, but as your in charge i will await your decision.
And what are we discard, we are just not processing THE SAME DATA over and over again.
>
>>
>>
>> >
>> >>
>> >> >
>> >> >[...]
>> >> >> >> +
>> >> >> >> +static int parse_section_header(SectionHeader
> *h,
>> >> >> >> + const uint8_t
> **pp,
>> > const uint8_t *p_end);
>> >> >> >> +
>> >> >> >> +typedef void SectionCallback(MpegTSFilter *f,
> const
>> > uint8_t *buf, const
>> >> >> > uint8_t *buf_end, SectionHeader *h);
>> >> >> >>
>> >> >> >> typedef void SetServiceCallback(void *opaque,
> int ret);
>> >> >> >>
>> >> >> >> @@ -76,11 +87,14 @@ struct MpegTSFilter {
>> >> >> >> int pid;
>> >> >> >> int es_id;
>> >> >> >> int last_cc; /* last cc code (-1 if first
> packet)
>> > */
>> >> >> >> + uint8_t last_version;
>> >> >> >> enum MpegTSFilterType type;
>> >> >> >> union {
>> >> >> >> MpegTSPESFilter pes_filter;
>> >> >> >> MpegTSSectionFilter section_filter;
>> >> >> >> } u;
>> >> >> >> + const char *name;
>> >> >> >> + uint8_t tid;
>> >> >> >> };
>> >> >> >>
>> >> >> >> #define MAX_PIDS_PER_PROGRAM 64
>> >> >> >> @@ -318,18 +332,45 @@ static void
>> > write_section_data(AVFormatContext *s,
>> >> >> > MpegTSFilter *tss1,
>> >> >> >> }else
>> >> >> >> crc_valid = 2;
>> >> >> >> }
>> >> >> >> - if (crc_valid)
>> >> >> >> - tss->section_cb(tss1,
>> > tss->section_buf,
>> >> >> > tss->section_h_size);
>> >> >> >> + if (crc_valid) {
>> >> >> >> + const uint8_t *p_end;
>> >> >> >> + const uint8_t *p;
>> >> >> >> + SectionHeader h;
>> >> >> >> +
>> >> >> >> + av_dlog(ts->stream, "%s:
> len
>> > %i\n",
>> >> >> > tss1->name, tss->section_h_size);
>> >> >> >> + hex_dump_debug(ts->stream,
>> > tss->section_buf,
>> >> >> > tss->section_h_size);
>> >> >> >> +
>> >> >> >> + p = tss->section_buf;
>> >> >> >> + p_end = p +
> tss->section_h_size - 4;
>> >> >> >> +
>> >> >> >> + if (parse_section_header(&h,
> &p,
>> > p_end) < 0)
>> >> >> >> + return;
>> >> >> >> +
>> >> >> >> + if (h.tid != tss1->tid)
>> >> >> >> + return;
>> >> >> >
>> >> >> > changing tid and parse_section_header handling
> belongs to 2
>> > seperate
>> >> >> > patches (and why are these changes needed at all?)
>> >> >> >
>> >> >> I have moved similar code that is used in all callbacks
> so it is
>> > done in this routine, if we go one to add more tables this will
> function for
>> > them as well.
>> >> >>
>> >> >> >
>> >> >> >> +
>> >> >> >> + // Check Version Numbers
>> >> >> >> + if
> (tss1->last_version==h.version)
>> >> >> >> + return;
>> >> >> >> + tss1->last_version =
> h.version;
>> >> >> >
>> >> >> > the last_version should only be updated after
> successfull
>> > parsing of
>> >> >> > the data
>> >> >>
>> >> >> There is no error return from the section callback, so
> there is no
>> > way to know if it is a valid decode!
>> >> >
>> >> >If iam not missing anything then what your patch does can be
>> >> >implemented by adding a if(version != last) into the existing
> code.
>> >> >and a last=version at the end of the existing code. Which
> does consider
>> >> >the successfull parsing.
>> >> >
>> >>
>> >> yes you could if you want to have to modify 4 functions with the
> SAME code
>> > each time (BLOAT), i tried, really tried to make this simpler so more
>> > modifications will not need FOUR FUNCTIONS modifying each time, but
> hey its you
>> > call.
>> >
>> > Maybe we misunderstand each other
>> > I pointed at a bug in your patch (that it updates the version
>> > independant of the success or failure of parsing a section).
>> > And i understood your reply as not knowing or caring how to fix it.
>> > My reply was just to show a easy way to fix it, NOT that i consider
>> > duplicating 3 lines of code 3 times the best solution.
>>
>>
>> Ok I will add in error management to the process routine and only update
> the version if it passes. Is this what you want
>
> that solves this problem, the more fundamental issue that the whole
> doesnt work in some cases (like concatenated streams) remains.
>
>
>>
>>
>> >>
>> >> >Also it is uncertain if such version checking itself is safe
> with real
>> >> >TS streams "out there". At the least it is missing
> seek
>> > handling and
>> >>
>> >> Ok on seek all that is needed is to mark the last_version to -1
> for all
>> > PIDS being watched,
>> >
>> > I think all versions have to be reset, not just all watched.
>> >
>> >
>> >> but this would then need to be able to change the input streams
> further
>> > down the chain, that is if there is a change in pids etc
>> >>
>> >>
>> >> >needs to be tested with concatenated streams that have
> matching
>> >> >versions but different content. (Theres a 1 in 32 change that
> 2 random
>> >> >concatenated streams will have matching versions, we cant
> just fail
>> >> >in 1 out of 32 cases)
>> >>
>> >>
>> >> Well if you want FFMPEG to work outside of the DVB SPEC, which
> says if a
>> > version number changes from one table to another then it is taken that
> this is a
>> > NEW table, then you need to work a way around this. Concatening dvb
> streams will
>> > always have this problem unless you remap all pids, tables, pcr etc,
> really a
>> > proper remux
>> >
>> > ffmpeg is supposed to be able to do this remux, there are some users
>> > using it for that, it would be bad if we lost this feature
>>
>>
>> Again we either process xxxx thousand sections or we do as the spec says
> and process only new/updated?
>
> The spec doesnt say that, it says:
> d) version_number:
> - When the characteristics of the TS described in the SI given in the
> present document change (e.g. new
> events start, different composition of elementary streams for a given
> service), then new SI data shall be
> sent containing the updated information. A new version of the SI data
> is signalled by sending a sub_table
> with the same identifiers as the previous sub_table containing the
> relevant data, but with the next value
> of version_number.
> - For the SI tables specified in the present document, the version_number
> applies to all sections of a
> sub_table.
>
> That speaks about "when X then ... shall be sent ...", sending happens
> on the muxer side, not the demuxer. The text quoted says nothing about
> what a demuxer should or should not do with what it receives. It
> simply describes what a demuxer can expect from a valid DVB stream.
> A concatenated stream as you already explained is not a valid DVB
> stream ...
So your now saying that the spec is only for the muxer, can you show me the spec for the demuxer then that is different, again i come back to the "New version .... but with the next value of version number". This spec if for DVB compatible systems, both muxers and demuxers.
So have you tested what happens when you play a concatenated stream on a DVB compliant receiver, you will get the same as with my modifications. As you always seem to tell people, fix the problem 1st. The only problem i can see is that you want the decoder NOT to follow the spec, but instead work for the NON COMPATIBLE streams, when instead the concatenate should be fixed to produce correct TS streams.
I will leave it now and let you decide, as this circle will just keep going round and round. If it will be included in any future ffmpeg release then great if not then i will just keep it in my local tree.
joolz
More information about the ffmpeg-devel
mailing list