diff mbox series

[FFmpeg-devel] avformat/tty: add probe function

Message ID 20200127205459.22590-1-onemda@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avformat/tty: add probe function
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork fail Make fate failed

Commit Message

Paul B Mahol Jan. 27, 2020, 8:54 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/tty.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Carl Eugen Hoyos Jan. 27, 2020, 9:32 p.m. UTC | #1
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
Paul B Mahol Jan. 27, 2020, 9:43 p.m. UTC | #2
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".
Carl Eugen Hoyos Jan. 27, 2020, 9:53 p.m. UTC | #3
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
Paul B Mahol Jan. 27, 2020, 9:55 p.m. UTC | #4
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".
Carl Eugen Hoyos Jan. 27, 2020, 10:03 p.m. UTC | #5
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 mbox series

Patch

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