| Message ID | 20200127205459.22590-1-onemda@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avformat/tty: add probe function | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | fail | Make fate failed |
Am Mo., 27. Jan. 2020 um 22:01 Uhr schrieb Paul B Mahol <onemda@gmail.com>: > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavformat/tty.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/libavformat/tty.c b/libavformat/tty.c > index 8d48f2c45c..acc6da27cc 100644 > --- a/libavformat/tty.c > +++ b/libavformat/tty.c > @@ -24,6 +24,8 @@ > * Tele-typewriter demuxer > */ > > +#include <ctype.h> > + > #include "libavutil/intreadwrite.h" > #include "libavutil/avstring.h" > #include "libavutil/log.h" > @@ -42,6 +44,16 @@ typedef struct TtyDemuxContext { > AVRational framerate; /**< Set by a private option. */ > } TtyDemuxContext; > > +static int read_probe(const AVProbeData *p) > +{ > + int64_t cnt = 0; > + > + for (int i = 0; i < p->buf_size; i++) > + cnt += !!isalnum(p->buf[i]); > + > + return cnt * 100 / p->buf_size; The function looks as if it could return scores higher than SCORE_MAX and as if it would return high scores for very small probe size. Both are not good, normally we test for both a minimum number of counts and a high percentage of correct values. > +} > + > /** > * Parse EFI header > */ > @@ -153,6 +165,7 @@ AVInputFormat ff_tty_demuxer = { > .name = "tty", > .long_name = NULL_IF_CONFIG_SMALL("Tele-typewriter"), > .priv_data_size = sizeof(TtyDemuxContext), > + .read_probe = read_probe, > .read_header = read_header, > .read_packet = read_packet, > .extensions = "ans,art,asc,diz,ice,nfo,txt,vt", You could add the extensions to above test. Background: An mpv user uploaded a jpg sample to the mpv bug tracker and renamed it to sample1.txt. Because of its high resolution and because the jpg probe function tries to distinguish from mjpeg (which only works by checking if EOI is at EOF) the extension returns a higher score than the jpg probe (the probe function works fine if I remove the filename suffix), So another fix would be to merge the mjpeg demuxer with the jpeg_pipe demuxer, I don't remember atm why I didn't do that. Carl Eugen
On 1/27/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Mo., 27. Jan. 2020 um 22:01 Uhr schrieb Paul B Mahol <onemda@gmail.com>: >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavformat/tty.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/libavformat/tty.c b/libavformat/tty.c >> index 8d48f2c45c..acc6da27cc 100644 >> --- a/libavformat/tty.c >> +++ b/libavformat/tty.c >> @@ -24,6 +24,8 @@ >> * Tele-typewriter demuxer >> */ >> >> +#include <ctype.h> >> + >> #include "libavutil/intreadwrite.h" >> #include "libavutil/avstring.h" >> #include "libavutil/log.h" >> @@ -42,6 +44,16 @@ typedef struct TtyDemuxContext { >> AVRational framerate; /**< Set by a private option. */ >> } TtyDemuxContext; >> >> +static int read_probe(const AVProbeData *p) >> +{ >> + int64_t cnt = 0; >> + >> + for (int i = 0; i < p->buf_size; i++) >> + cnt += !!isalnum(p->buf[i]); >> + >> + return cnt * 100 / p->buf_size; > > The function looks as if it could return scores > higher than SCORE_MAX and as if it would > return high scores for very small probe size. > Both are not good, normally we test for both > a minimum number of counts and a high > percentage of correct values. Why are you propagandizing untrue statements here? Score returned by this probe can not be bigger than 100 and less than 0. Probe function works fine and adding extension is pointless. > >> +} >> + >> /** >> * Parse EFI header >> */ >> @@ -153,6 +165,7 @@ AVInputFormat ff_tty_demuxer = { >> .name = "tty", >> .long_name = NULL_IF_CONFIG_SMALL("Tele-typewriter"), >> .priv_data_size = sizeof(TtyDemuxContext), >> + .read_probe = read_probe, >> .read_header = read_header, >> .read_packet = read_packet, >> .extensions = "ans,art,asc,diz,ice,nfo,txt,vt", > > You could add the extensions to above test. > > Background: An mpv user uploaded a jpg sample to the mpv > bug tracker and renamed it to sample1.txt. Because of its high > resolution and because the jpg probe function tries to distinguish > from mjpeg (which only works by checking if EOI is at EOF) the > extension returns a higher score than the jpg probe (the probe > function works fine if I remove the filename suffix), > > So another fix would be to merge the mjpeg demuxer with the > jpeg_pipe demuxer, I don't remember atm why I didn't do that. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Am Mo., 27. Jan. 2020 um 22:44 Uhr schrieb Paul B Mahol <onemda@gmail.com>: > > On 1/27/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Mo., 27. Jan. 2020 um 22:01 Uhr schrieb Paul B Mahol <onemda@gmail.com>: > >> > >> Signed-off-by: Paul B Mahol <onemda@gmail.com> > >> --- > >> libavformat/tty.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/libavformat/tty.c b/libavformat/tty.c > >> index 8d48f2c45c..acc6da27cc 100644 > >> --- a/libavformat/tty.c > >> +++ b/libavformat/tty.c > >> @@ -24,6 +24,8 @@ > >> * Tele-typewriter demuxer > >> */ > >> > >> +#include <ctype.h> > >> + > >> #include "libavutil/intreadwrite.h" > >> #include "libavutil/avstring.h" > >> #include "libavutil/log.h" > >> @@ -42,6 +44,16 @@ typedef struct TtyDemuxContext { > >> AVRational framerate; /**< Set by a private option. */ > >> } TtyDemuxContext; > >> > >> +static int read_probe(const AVProbeData *p) > >> +{ > >> + int64_t cnt = 0; > >> + > >> + for (int i = 0; i < p->buf_size; i++) > >> + cnt += !!isalnum(p->buf[i]); > >> + > >> + return cnt * 100 / p->buf_size; > > > > The function looks as if it could return scores > > higher than SCORE_MAX and as if it would > > return high scores for very small probe size. > > Both are not good, normally we test for both > > a minimum number of counts and a high > > percentage of correct values. > > Why are you propagandizing untrue statements here? Well, it is true that the function looked to me as if it could return scores > 100 (which it does not). > Score returned by this probe can not be bigger than 100 and less than 0. It can return 100 for very small probe size which is not good. > Probe function works fine and adding extension is pointless. You could only return a high score if your probe heuristic agrees and if the extension matches. > >> +} > >> + > >> /** > >> * Parse EFI header > >> */ > >> @@ -153,6 +165,7 @@ AVInputFormat ff_tty_demuxer = { > >> .name = "tty", > >> .long_name = NULL_IF_CONFIG_SMALL("Tele-typewriter"), > >> .priv_data_size = sizeof(TtyDemuxContext), > >> + .read_probe = read_probe, > >> .read_header = read_header, > >> .read_packet = read_packet, > >> .extensions = "ans,art,asc,diz,ice,nfo,txt,vt", > > > > You could add the extensions to above test. > > > > Background: An mpv user uploaded a jpg sample to the mpv > > bug tracker and renamed it to sample1.txt. Because of its high > > resolution and because the jpg probe function tries to distinguish > > from mjpeg (which only works by checking if EOI is at EOF) the > > extension returns a higher score than the jpg probe (the probe > > function works fine if I remove the filename suffix), > > > > So another fix would be to merge the mjpeg demuxer with the > > jpeg_pipe demuxer, I don't remember atm why I didn't do that. Carl Eugen
On 1/27/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Mo., 27. Jan. 2020 um 22:44 Uhr schrieb Paul B Mahol <onemda@gmail.com>: >> >> On 1/27/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> > Am Mo., 27. Jan. 2020 um 22:01 Uhr schrieb Paul B Mahol >> > <onemda@gmail.com>: >> >> >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> >> --- >> >> libavformat/tty.c | 13 +++++++++++++ >> >> 1 file changed, 13 insertions(+) >> >> >> >> diff --git a/libavformat/tty.c b/libavformat/tty.c >> >> index 8d48f2c45c..acc6da27cc 100644 >> >> --- a/libavformat/tty.c >> >> +++ b/libavformat/tty.c >> >> @@ -24,6 +24,8 @@ >> >> * Tele-typewriter demuxer >> >> */ >> >> >> >> +#include <ctype.h> >> >> + >> >> #include "libavutil/intreadwrite.h" >> >> #include "libavutil/avstring.h" >> >> #include "libavutil/log.h" >> >> @@ -42,6 +44,16 @@ typedef struct TtyDemuxContext { >> >> AVRational framerate; /**< Set by a private option. */ >> >> } TtyDemuxContext; >> >> >> >> +static int read_probe(const AVProbeData *p) >> >> +{ >> >> + int64_t cnt = 0; >> >> + >> >> + for (int i = 0; i < p->buf_size; i++) >> >> + cnt += !!isalnum(p->buf[i]); >> >> + >> >> + return cnt * 100 / p->buf_size; >> > >> > The function looks as if it could return scores >> > higher than SCORE_MAX and as if it would >> > return high scores for very small probe size. >> > Both are not good, normally we test for both >> > a minimum number of counts and a high >> > percentage of correct values. >> >> Why are you propagandizing untrue statements here? > > Well, it is true that the function looked to me as if > it could return scores > 100 (which it does not). > >> Score returned by this probe can not be bigger than 100 and less than 0. > > It can return 100 for very small probe size which is not good. No it can not, check your math skills again. I expected you would give more constructive review. > >> Probe function works fine and adding extension is pointless. > > You could only return a high score if your probe heuristic > agrees and if the extension matches. > >> >> +} >> >> + >> >> /** >> >> * Parse EFI header >> >> */ >> >> @@ -153,6 +165,7 @@ AVInputFormat ff_tty_demuxer = { >> >> .name = "tty", >> >> .long_name = NULL_IF_CONFIG_SMALL("Tele-typewriter"), >> >> .priv_data_size = sizeof(TtyDemuxContext), >> >> + .read_probe = read_probe, >> >> .read_header = read_header, >> >> .read_packet = read_packet, >> >> .extensions = "ans,art,asc,diz,ice,nfo,txt,vt", >> > >> > You could add the extensions to above test. >> > >> > Background: An mpv user uploaded a jpg sample to the mpv >> > bug tracker and renamed it to sample1.txt. Because of its high >> > resolution and because the jpg probe function tries to distinguish >> > from mjpeg (which only works by checking if EOI is at EOF) the >> > extension returns a higher score than the jpg probe (the probe >> > function works fine if I remove the filename suffix), >> > >> > So another fix would be to merge the mjpeg demuxer with the >> > jpeg_pipe demuxer, I don't remember atm why I didn't do that. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Am Mo., 27. Jan. 2020 um 22:56 Uhr schrieb Paul B Mahol <onemda@gmail.com>: > > On 1/27/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Mo., 27. Jan. 2020 um 22:44 Uhr schrieb Paul B Mahol <onemda@gmail.com>: > >> > >> On 1/27/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> > Am Mo., 27. Jan. 2020 um 22:01 Uhr schrieb Paul B Mahol > >> > <onemda@gmail.com>: > >> >> > >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> > >> >> --- > >> >> libavformat/tty.c | 13 +++++++++++++ > >> >> 1 file changed, 13 insertions(+) > >> >> > >> >> diff --git a/libavformat/tty.c b/libavformat/tty.c > >> >> index 8d48f2c45c..acc6da27cc 100644 > >> >> --- a/libavformat/tty.c > >> >> +++ b/libavformat/tty.c > >> >> @@ -24,6 +24,8 @@ > >> >> * Tele-typewriter demuxer > >> >> */ > >> >> > >> >> +#include <ctype.h> > >> >> + > >> >> #include "libavutil/intreadwrite.h" > >> >> #include "libavutil/avstring.h" > >> >> #include "libavutil/log.h" > >> >> @@ -42,6 +44,16 @@ typedef struct TtyDemuxContext { > >> >> AVRational framerate; /**< Set by a private option. */ > >> >> } TtyDemuxContext; > >> >> > >> >> +static int read_probe(const AVProbeData *p) > >> >> +{ > >> >> + int64_t cnt = 0; > >> >> + > >> >> + for (int i = 0; i < p->buf_size; i++) > >> >> + cnt += !!isalnum(p->buf[i]); > >> >> + > >> >> + return cnt * 100 / p->buf_size; > >> > > >> > The function looks as if it could return scores > >> > higher than SCORE_MAX and as if it would > >> > return high scores for very small probe size. > >> > Both are not good, normally we test for both > >> > a minimum number of counts and a high > >> > percentage of correct values. > >> > >> Why are you propagandizing untrue statements here? > > > > Well, it is true that the function looked to me as if > > it could return scores > 100 (which it does not). > > > >> Score returned by this probe can not be bigger than 100 and less than 0. > > > > It can return 100 for very small probe size which is not good. > > No it can not, check your math skills again. To elaborate: It can return 100 for a probe size of 32 which I think is not ideal. Carl Eugen
diff --git a/libavformat/tty.c b/libavformat/tty.c index 8d48f2c45c..acc6da27cc 100644 --- a/libavformat/tty.c +++ b/libavformat/tty.c @@ -24,6 +24,8 @@ * Tele-typewriter demuxer */ +#include <ctype.h> + #include "libavutil/intreadwrite.h" #include "libavutil/avstring.h" #include "libavutil/log.h" @@ -42,6 +44,16 @@ typedef struct TtyDemuxContext { AVRational framerate; /**< Set by a private option. */ } TtyDemuxContext; +static int read_probe(const AVProbeData *p) +{ + int64_t cnt = 0; + + for (int i = 0; i < p->buf_size; i++) + cnt += !!isalnum(p->buf[i]); + + return cnt * 100 / p->buf_size; +} + /** * Parse EFI header */ @@ -153,6 +165,7 @@ AVInputFormat ff_tty_demuxer = { .name = "tty", .long_name = NULL_IF_CONFIG_SMALL("Tele-typewriter"), .priv_data_size = sizeof(TtyDemuxContext), + .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, .extensions = "ans,art,asc,diz,ice,nfo,txt,vt",
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavformat/tty.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)