[FFmpeg-devel] New patch for mpegts.c
Michael Niedermayer
michaelni at gmx.at
Wed Oct 17 01:41:42 CEST 2012
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.
>
> >
> >[...]
> >> >> +
> >> >> +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.
>
> >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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121017/87e0c13e/attachment.asc>
More information about the ffmpeg-devel
mailing list