Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On 30.08.2014, at 15:38, wm4 nfx...@googlemail.com wrote: +// The packet-size is stored as part of the packet. +if ((ret = avio_read(s-pb, tmp, 3)) 0) +return ret; + +len = AV_RB16(tmp + 1); + +if ((ret = av_new_packet(pkt, len + 3)) 0) +return ret; + +memcpy(pkt-data, tmp, 3); + +if ((ret = avio_read(s-pb, pkt-data + 3, len)) 0) { +av_free_packet(pkt); +return ret; +} I think this will not handle short reads correctly, retuning uninitialised data. My suggestion would be to read the length, then seek back (buffering should ensure this is no issue even if we read from stdin) and then use the functions to read the full packet with all the proper error handling. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, Aug 31, 2014 at 9:41 AM, Reimar Döffinger reimar.doeffin...@gmx.de wrote: On 30.08.2014, at 15:38, wm4 nfx...@googlemail.com wrote: +// The packet-size is stored as part of the packet. +if ((ret = avio_read(s-pb, tmp, 3)) 0) +return ret; + +len = AV_RB16(tmp + 1); + +if ((ret = av_new_packet(pkt, len + 3)) 0) +return ret; + +memcpy(pkt-data, tmp, 3); + +if ((ret = avio_read(s-pb, pkt-data + 3, len)) 0) { +av_free_packet(pkt); +return ret; +} I think this will not handle short reads correctly, retuning uninitialised data. My suggestion would be to read the length, then seek back (buffering should ensure this is no issue even if we read from stdin) and then use the functions to read the full packet with all the proper error handling. I don't see a problem in the code he proposed. It seems to handle every read with an error check, without relying on potential buffering of data to allow a seek backwards. This is assuming avio_read does return an error if it runs against EOF, which I assume it does? I didn't double check that. What exactly do you think is wrong here? - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, Aug 31, 2014 at 10:15:56AM +0200, Hendrik Leppkes wrote: On Sun, Aug 31, 2014 at 9:41 AM, Reimar Döffinger reimar.doeffin...@gmx.de wrote: On 30.08.2014, at 15:38, wm4 nfx...@googlemail.com wrote: +// The packet-size is stored as part of the packet. +if ((ret = avio_read(s-pb, tmp, 3)) 0) +return ret; + +len = AV_RB16(tmp + 1); + +if ((ret = av_new_packet(pkt, len + 3)) 0) +return ret; + +memcpy(pkt-data, tmp, 3); + +if ((ret = avio_read(s-pb, pkt-data + 3, len)) 0) { +av_free_packet(pkt); +return ret; +} I think this will not handle short reads correctly, retuning uninitialised data. My suggestion would be to read the length, then seek back (buffering should ensure this is no issue even if we read from stdin) and then use the functions to read the full packet with all the proper error handling. I don't see a problem in the code he proposed. It seems to handle every read with an error check, without relying on potential buffering of data to allow a seek backwards. This is assuming avio_read does return an error if it runs against EOF, which I assume it does? I didn't double check that. Why would it? That would make it a huge pain to read formats where you don't know ahead of time how long they are (e.g. streaming TS files via stdin). What exactly do you think is wrong here? The code does not handle 0 ret len (I think 0 should not be possible), plus it is complex and I am not at all confident it's the only case it misses. There is a reason we have helper functions. If you absolutely do not want to seek back, there is also the option of reading a 3-byte packet (but then like now you have to add handling getting fewer than that) and then use the function to expand the packet to read the rest. However there is a good reason why seeking back small amounts is _supposed_ to work always, 100% reliable, and it should be fine to take advantage of it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, 31 Aug 2014 10:24:13 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 10:15:56AM +0200, Hendrik Leppkes wrote: On Sun, Aug 31, 2014 at 9:41 AM, Reimar Döffinger reimar.doeffin...@gmx.de wrote: On 30.08.2014, at 15:38, wm4 nfx...@googlemail.com wrote: +// The packet-size is stored as part of the packet. +if ((ret = avio_read(s-pb, tmp, 3)) 0) +return ret; + +len = AV_RB16(tmp + 1); + +if ((ret = av_new_packet(pkt, len + 3)) 0) +return ret; + +memcpy(pkt-data, tmp, 3); + +if ((ret = avio_read(s-pb, pkt-data + 3, len)) 0) { +av_free_packet(pkt); +return ret; +} I think this will not handle short reads correctly, retuning uninitialised data. My suggestion would be to read the length, then seek back (buffering should ensure this is no issue even if we read from stdin) and then use the functions to read the full packet with all the proper error handling. I don't see a problem in the code he proposed. It seems to handle every read with an error check, without relying on potential buffering of data to allow a seek backwards. This is assuming avio_read does return an error if it runs against EOF, which I assume it does? I didn't double check that. Why would it? That would make it a huge pain to read formats where you don't know ahead of time how long they are (e.g. streaming TS files via stdin). What exactly do you think is wrong here? The code does not handle 0 ret len (I think 0 should not be possible), plus it is complex and I am not at all confident it's the only case it misses. There is a reason we have helper functions. If you absolutely do not want to seek back, there is also the option of reading a 3-byte packet (but then like now you have to add handling getting fewer than that) and then use the function to expand the packet to read the rest. However there is a good reason why seeking back small amounts is _supposed_ to work always, 100% reliable, and it should be fine to take advantage of it. When I looked at avio_read, it seemed like it doesn't allow short reads, so I simplified the error checks. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
--- I got the semantics of avio_read wrong: short reads on EOF are in fact allowed and don't return an error. Hopefully this is correct now... --- libavformat/Makefile | 1 + libavformat/allformats.c | 1 + libavformat/supdec.c | 92 3 files changed, 94 insertions(+) create mode 100644 libavformat/supdec.c diff --git a/libavformat/Makefile b/libavformat/Makefile index 3d124fb..b4965fe 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -405,6 +405,7 @@ OBJS-$(CONFIG_SRT_MUXER) += srtenc.o OBJS-$(CONFIG_STR_DEMUXER) += psxstr.o OBJS-$(CONFIG_SUBVIEWER1_DEMUXER)+= subviewer1dec.o subtitles.o OBJS-$(CONFIG_SUBVIEWER_DEMUXER) += subviewerdec.o subtitles.o +OBJS-$(CONFIG_SUP_DEMUXER) += supdec.o OBJS-$(CONFIG_SWF_DEMUXER) += swfdec.o swf.o OBJS-$(CONFIG_SWF_MUXER) += swfenc.o swf.o OBJS-$(CONFIG_TAK_DEMUXER) += takdec.o apetag.o img2.o rawdec.o diff --git a/libavformat/allformats.c b/libavformat/allformats.c index 8f70c4b..e6c0e5f 100644 --- a/libavformat/allformats.c +++ b/libavformat/allformats.c @@ -280,6 +280,7 @@ void av_register_all(void) REGISTER_DEMUXER (STR, str); REGISTER_DEMUXER (SUBVIEWER1, subviewer1); REGISTER_DEMUXER (SUBVIEWER,subviewer); +REGISTER_DEMUXER (SUP, sup); REGISTER_MUXDEMUX(SWF, swf); REGISTER_DEMUXER (TAK, tak); REGISTER_MUXER (TEE, tee); diff --git a/libavformat/supdec.c b/libavformat/supdec.c new file mode 100644 index 000..1b4ebb1 --- /dev/null +++ b/libavformat/supdec.c @@ -0,0 +1,92 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include avformat.h +#include internal.h +#include libavutil/intreadwrite.h + +#define SUP_PGS_MAGIC 0x5047 /* PG, big endian */ + +static int sup_read_header(AVFormatContext *s) +{ +AVStream *st = avformat_new_stream(s, NULL); +if (!st) +return AVERROR(ENOMEM); +st-codec-codec_type = AVMEDIA_TYPE_SUBTITLE; +st-codec-codec_id = AV_CODEC_ID_HDMV_PGS_SUBTITLE; +avpriv_set_pts_info(st, 32, 1, 9); + +return 0; +} + +static int sup_read_packet(struct AVFormatContext *s, AVPacket *pkt) +{ +char tmp[3]; +size_t len; +int64_t pts, pos; +int ret; + +pos = avio_tell(s-pb); + +if (avio_rb16(s-pb) != SUP_PGS_MAGIC) +return avio_feof(s-pb) ? AVERROR_EOF : AVERROR_INVALIDDATA; + +pts = avio_rb32(s-pb); +avio_rb32(s-pb); /* discard DTS (usually 0, and useless) */ + +// The packet-size is stored as part of the packet. +if ((ret = avio_read(s-pb, tmp, 3)) 3) +return ret 0 ? ret : AVERROR_INVALIDDATA; + +len = AV_RB16(tmp + 1); + +if ((ret = av_new_packet(pkt, len + 3)) 0) +return ret; + +memcpy(pkt-data, tmp, 3); + +if ((ret = avio_read(s-pb, pkt-data + 3, len)) len) { +av_free_packet(pkt); +return ret 0 ? ret : AVERROR_INVALIDDATA; +} + +pkt-stream_index = 0; +pkt-flags |= AV_PKT_FLAG_KEY; +pkt-pos = pos; +pkt-pts = pts; + +return 0; +} + +static int sup_probe(AVProbeData *p) +{ +if (p-buf_size 2 || memcmp(p-buf, PG, 2)) +return 0; +return AVPROBE_SCORE_EXTENSION; +} + +AVInputFormat ff_sup_demuxer = { +.name = sup, +.long_name = NULL_IF_CONFIG_SMALL(raw HDMV Presentation Graphic Stream subtitles), +.extensions = sup, +.mime_type = application/x-pgs, +.read_probe = sup_probe, +.read_header= sup_read_header, +.read_packet= sup_read_packet, +.flags = AVFMT_GENERIC_INDEX, +}; -- 2.1.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, Aug 31, 2014 at 09:41:49AM +0200, Reimar Döffinger wrote: On 30.08.2014, at 15:38, wm4 nfx...@googlemail.com wrote: +// The packet-size is stored as part of the packet. +if ((ret = avio_read(s-pb, tmp, 3)) 0) +return ret; + +len = AV_RB16(tmp + 1); + +if ((ret = av_new_packet(pkt, len + 3)) 0) +return ret; + +memcpy(pkt-data, tmp, 3); + +if ((ret = avio_read(s-pb, pkt-data + 3, len)) 0) { +av_free_packet(pkt); +return ret; +} I think this will not handle short reads correctly, retuning uninitialised data. My suggestion would be to read the length, then seek back (buffering should ensure this is no issue even if we read from stdin) and then use the functions to read the full packet with all the proper error handling. note, for guranteed seekback on non seekable input ffio_ensure_seekback() is needed [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, 31 Aug 2014 11:50:36 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 11:39:16AM +0200, wm4 wrote: I got the semantics of avio_read wrong: short reads on EOF are in fact allowed and don't return an error. Hopefully this is correct now... What I _really_ dislike about these approaches is that we hard-code EOF behaviour in every single demuxer. So some will return partial packets some won't. And if we wanted to make that behaviour configurable we'd have to add options to every single one instead of changing the code in one single place. Not to mention all the other optimizations, just look how much e.g. av_get_packet actually does! Why do you absolutely want to handle this manually via avio_read? You can just use av_get_packet + av_append_packet, then you only need to check the size once, and you also don't have to set pkt-pos manually and you still don't have to do seekback. I wasn't aware of av_get/append_packet() (sorry if it was mentioned before and I ignored it) - it looks nice. I'll try to use it and send a patch again later. It's a bit not-nice that it reallocs the packet, but there's probably no way it matters in this case at all. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, Aug 31, 2014 at 12:00:13PM +0200, wm4 wrote: On Sun, 31 Aug 2014 11:50:36 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 11:39:16AM +0200, wm4 wrote: I got the semantics of avio_read wrong: short reads on EOF are in fact allowed and don't return an error. Hopefully this is correct now... What I _really_ dislike about these approaches is that we hard-code EOF behaviour in every single demuxer. So some will return partial packets some won't. And if we wanted to make that behaviour configurable we'd have to add options to every single one instead of changing the code in one single place. Not to mention all the other optimizations, just look how much e.g. av_get_packet actually does! Why do you absolutely want to handle this manually via avio_read? You can just use av_get_packet + av_append_packet, then you only need to check the size once, and you also don't have to set pkt-pos manually and you still don't have to do seekback. I wasn't aware of av_get/append_packet() (sorry if it was mentioned before and I ignored it) I mentioned it but was too lazy to find out the exact function names. So I'll take part of the blame if you missed it :) - it looks nice. I'll try to use it and send a patch again later. It's a bit not-nice that it reallocs the packet, but there's probably no way it matters in this case at all. I agree. _If_ it mattered at some point it would be possible to have a variant that specifies an initial allocation size - for many cases it would be possible to reasonably estimate the final packet size. Though especially for this case that would be just overkill anyway and I think going for as simple as possible is best. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
--- Following the advice to use av_get_packet() etc. Tese functions still return partial packets, but mark them as corrupted - so returning them should be fine. --- libavformat/Makefile | 1 + libavformat/allformats.c | 1 + libavformat/supdec.c | 85 3 files changed, 87 insertions(+) create mode 100644 libavformat/supdec.c diff --git a/libavformat/Makefile b/libavformat/Makefile index 3d124fb..b4965fe 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -405,6 +405,7 @@ OBJS-$(CONFIG_SRT_MUXER) += srtenc.o OBJS-$(CONFIG_STR_DEMUXER) += psxstr.o OBJS-$(CONFIG_SUBVIEWER1_DEMUXER)+= subviewer1dec.o subtitles.o OBJS-$(CONFIG_SUBVIEWER_DEMUXER) += subviewerdec.o subtitles.o +OBJS-$(CONFIG_SUP_DEMUXER) += supdec.o OBJS-$(CONFIG_SWF_DEMUXER) += swfdec.o swf.o OBJS-$(CONFIG_SWF_MUXER) += swfenc.o swf.o OBJS-$(CONFIG_TAK_DEMUXER) += takdec.o apetag.o img2.o rawdec.o diff --git a/libavformat/allformats.c b/libavformat/allformats.c index 8f70c4b..e6c0e5f 100644 --- a/libavformat/allformats.c +++ b/libavformat/allformats.c @@ -280,6 +280,7 @@ void av_register_all(void) REGISTER_DEMUXER (STR, str); REGISTER_DEMUXER (SUBVIEWER1, subviewer1); REGISTER_DEMUXER (SUBVIEWER,subviewer); +REGISTER_DEMUXER (SUP, sup); REGISTER_MUXDEMUX(SWF, swf); REGISTER_DEMUXER (TAK, tak); REGISTER_MUXER (TEE, tee); diff --git a/libavformat/supdec.c b/libavformat/supdec.c new file mode 100644 index 000..920a7a0 --- /dev/null +++ b/libavformat/supdec.c @@ -0,0 +1,85 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include avformat.h +#include internal.h +#include libavutil/intreadwrite.h + +#define SUP_PGS_MAGIC 0x5047 /* PG, big endian */ + +static int sup_read_header(AVFormatContext *s) +{ +AVStream *st = avformat_new_stream(s, NULL); +if (!st) +return AVERROR(ENOMEM); +st-codec-codec_type = AVMEDIA_TYPE_SUBTITLE; +st-codec-codec_id = AV_CODEC_ID_HDMV_PGS_SUBTITLE; +avpriv_set_pts_info(st, 32, 1, 9); + +return 0; +} + +static int sup_read_packet(struct AVFormatContext *s, AVPacket *pkt) +{ +int64_t pts, pos; +int ret; + +pos = avio_tell(s-pb); + +if (avio_rb16(s-pb) != SUP_PGS_MAGIC) +return avio_feof(s-pb) ? AVERROR_EOF : AVERROR_INVALIDDATA; + +pts = avio_rb32(s-pb); +avio_rb32(s-pb); /* discard DTS (usually 0, and useless) */ + +if ((ret = av_get_packet(s-pb, pkt, 3)) 0) +return ret; + +pkt-stream_index = 0; +pkt-flags |= AV_PKT_FLAG_KEY; +pkt-pos = pos; +pkt-pts = pts; + +if (pkt-size = 3) { +// The full packet size is stored as part of the packet. +size_t len = AV_RB16(pkt-data + 1); + +if ((ret = av_append_packet(s-pb, pkt, len)) 0) +return ret; +} + +return 0; +} + +static int sup_probe(AVProbeData *p) +{ +if (p-buf_size 2 || memcmp(p-buf, PG, 2)) +return 0; +return AVPROBE_SCORE_EXTENSION; +} + +AVInputFormat ff_sup_demuxer = { +.name = sup, +.long_name = NULL_IF_CONFIG_SMALL(raw HDMV Presentation Graphic Stream subtitles), +.extensions = sup, +.mime_type = application/x-pgs, +.read_probe = sup_probe, +.read_header= sup_read_header, +.read_packet= sup_read_packet, +.flags = AVFMT_GENERIC_INDEX, +}; -- 2.1.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, 31 Aug 2014 00:33:58 +0200 wm4 nfx...@googlemail.com wrote: On Sat, 30 Aug 2014 18:16:27 -0400 Marcus Johnson bumblebritche...@gmail.com wrote: Here's a PGS subtitle sample from Eac3to: https://dl.dropboxusercontent.com/u/52358991/English.zip you saw the few samples we have in the repo, right? http://samples.ffmpeg.org/sub/BluRay/ http://samples.ffmpeg.org/mplayer-bugs/bug2176/ will try to dig some more. -compn ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, Aug 31, 2014 at 01:04:29PM +0200, wm4 wrote: +static int sup_probe(AVProbeData *p) +{ +if (p-buf_size 2 || memcmp(p-buf, PG, 2)) +return 0; +return AVPROBE_SCORE_EXTENSION; I understand if you consider it not worth the effort, but ideally this would scan ahead several packets, and if it all matches up return a higher score. The MP3 probe function is possibly a good reference (though this one should be bit simpler). ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, 31 Aug 2014 14:25:21 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 01:04:29PM +0200, wm4 wrote: +static int sup_probe(AVProbeData *p) +{ +if (p-buf_size 2 || memcmp(p-buf, PG, 2)) +return 0; +return AVPROBE_SCORE_EXTENSION; I understand if you consider it not worth the effort, but ideally this would scan ahead several packets, and if it all matches up return a higher score. The MP3 probe function is possibly a good reference (though this one should be bit simpler). Other formats are also relatively lazy and just check the magic atthe start of the file and call it a day (e.g. flac, some img2dec probers). Of course it's possible that 2 bytes (and ASCII) is a bit too prone to false positives, so maybe it should be improved. Since PGS packets can be only at most ~64KB big, I guess it would be feasible to check whether there is a second PGS packet after the first one. Would that be sufficient? In theory, it would be nice if the general probe code could jzst try to read a few packets... ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, Aug 31, 2014 at 02:57:54PM +0200, wm4 wrote: On Sun, 31 Aug 2014 14:25:21 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 01:04:29PM +0200, wm4 wrote: +static int sup_probe(AVProbeData *p) +{ +if (p-buf_size 2 || memcmp(p-buf, PG, 2)) +return 0; +return AVPROBE_SCORE_EXTENSION; I understand if you consider it not worth the effort, but ideally this would scan ahead several packets, and if it all matches up return a higher score. The MP3 probe function is possibly a good reference (though this one should be bit simpler). Other formats are also relatively lazy and just check the magic atthe start of the file and call it a day (e.g. flac, some img2dec probers). Of course it's possible that 2 bytes (and ASCII) is a bit too prone to false positives, so maybe it should be improved. Since PGS packets can be only at most ~64KB big, I guess it would be feasible to check whether there is a second PGS packet after the first one. Would that be sufficient? Ideally it would scan the whole probe buffer, and return a score based on how many consecutive packets it found compared to the probe size (more or less). tools/probetest is a useful tool, but so far it will only check if the score is above MAX/4, so demuxers (needlessly) crappy probe but also low score get a pass... In theory, it would be nice if the general probe code could jzst try to read a few packets... Probing is quite performance critical, especially since fringe formats can even impact the major ones (unless we start sorting them by how common they are and abort early - but that has its own long list of issues). So I think specialized code will remain necessary. However it might be possible to factor out common approaches, but that would need some careful checking. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, 31 Aug 2014 15:16:49 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 02:57:54PM +0200, wm4 wrote: On Sun, 31 Aug 2014 14:25:21 +0200 Reimar Döffinger reimar.doeffin...@gmx.de wrote: On Sun, Aug 31, 2014 at 01:04:29PM +0200, wm4 wrote: +static int sup_probe(AVProbeData *p) +{ +if (p-buf_size 2 || memcmp(p-buf, PG, 2)) +return 0; +return AVPROBE_SCORE_EXTENSION; I understand if you consider it not worth the effort, but ideally this would scan ahead several packets, and if it all matches up return a higher score. The MP3 probe function is possibly a good reference (though this one should be bit simpler). Other formats are also relatively lazy and just check the magic atthe start of the file and call it a day (e.g. flac, some img2dec probers). Of course it's possible that 2 bytes (and ASCII) is a bit too prone to false positives, so maybe it should be improved. Since PGS packets can be only at most ~64KB big, I guess it would be feasible to check whether there is a second PGS packet after the first one. Would that be sufficient? Ideally it would scan the whole probe buffer, and return a score based on how many consecutive packets it found compared to the probe size (more or less). tools/probetest is a useful tool, but so far it will only check if the score is above MAX/4, so demuxers (needlessly) crappy probe but also low score get a pass... Well yes, it would be possible to loop over the entire probe buffer, until it ends on a packet boundary, or there's a partial cut-off packet. How exactly would you suggest the probe score? Personally, I'd probably do the following: if the header of the first packet is available (i.e. PG magic), then return SCORE_EXTENSION-1. If the probe buffer is large enough to include the header of the next packet, and the next packet has no magic (i.e. probably invalid), return 0. Otherwise, return SCORE_EXTENSION+1. Suggestions? In theory, it would be nice if the general probe code could jzst try to read a few packets... Probing is quite performance critical, especially since fringe formats can even impact the major ones (unless we start sorting them by how common they are and abort early - but that has its own long list of issues). So I think specialized code will remain necessary. However it might be possible to factor out common approaches, but that would need some careful checking. Possibly it would make sense to provide a generic function, that creates an in-memory aviobuf, and lets the demuxer read packets from it. Demuxers which need it could use it in the probe function. But yeah, that probably is a bit out of the scope of this tiny patch. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sun, 31 Aug 2014 15:06:08 + (UTC) Carl Eugen Hoyos ceho...@ag.or.at wrote: wm4 nfxjfg at googlemail.com writes: +return AVPROBE_SCORE_EXTENSION; I understand if you consider it not worth the effort, but ideally this would scan ahead several packets, and if it all matches up return a higher score. The MP3 probe function is possibly a good reference (though this one should be bit simpler). I have written such a patch some time ago, so you can commit with AVPROBE_SCORE_EXTENSION / 2 (or / 4), I will try to improve using my older work if you want. (It was too slow so it has to be changed.) That would potentially be helpful, but I think I can finish this patch without it. Other formats are also relatively lazy Please point me to such a format, I would like to fix it. EXTENSION is ok for 32 bit, MAX is ok for 64bit. I think there is one bad example, but I cannot find it atm. E.g pictor_probe in img2dec.c. Though that returns AVPROBE_SCORE_EXTENSION / 4, which is certainly low enough (it's = AVPROBE_SCORE_RETRY, so the API user knows that it's unreliable). ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
compn tempn at mi.rr.com writes: will try to dig some more. sup samples are attached to ticket #2208, the original file is in http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2208/ Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sat, 30 Aug 2014 18:16:27 -0400 Marcus Johnson bumblebritche...@gmail.com wrote: Here's a PGS subtitle sample from Eac3to: https://dl.dropboxusercontent.com/u/52358991/English.zip Thanks! The DTS fields in this one are all 0 too. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer
On Sat, Aug 30, 2014 at 9:41 PM, Marcus Johnson bumblebritche...@gmail.com wrote: If you don't mind me asking, what is DTS? because when I hear it I think of the DTS audio codec, but I know it's obviously not that, and googling DTS in the codec context won't get me anywhere. DTS = decoding time stamp PTS = presentation time stamp See http://stackoverflow.com/questions/6044330/ffmpeg-c-what-are-pts-and-dts-what-does-this-code-block-do-in-ffmpeg-c Timothy ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel