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
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
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
