diff mbox

[FFmpeg-devel,V2] lavf/flvdec: Fix AMF NUMBER type to metadata lost precision

Message ID 1553779409-32200-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao March 28, 2019, 1:23 p.m. UTC 
From: Jun Zhao <barryjzhao@tencent.com>

Use %.12g replace %.f when save AMF NUMBER(double) type to
metadata. And update fate ref.

before this fix, we get FLV metadata like:

 Metadata:
    lasttimestamp   : 113
    lastkeyframetimestamp: 112

after this fix:

 Metadata:
    lasttimestamp   : 113.005
    lastkeyframetimestamp: 111.678

Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 libavformat/flvdec.c                  |    2 +-
 tests/ref/fate/flv-add_keyframe_index |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jun Zhao April 3, 2019, 1:22 a.m. UTC | #1 
On Thu, Mar 28, 2019 at 9:23 PM Jun Zhao <mypopydev@gmail.com> wrote:
>
> From: Jun Zhao <barryjzhao@tencent.com>
>
> Use %.12g replace %.f when save AMF NUMBER(double) type to
> metadata. And update fate ref.
>
> before this fix, we get FLV metadata like:
>
>  Metadata:
>     lasttimestamp   : 113
>     lastkeyframetimestamp: 112
>
> after this fix:
>
>  Metadata:
>     lasttimestamp   : 113.005
>     lastkeyframetimestamp: 111.678
>
> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> ---
>  libavformat/flvdec.c                  |    2 +-
>  tests/ref/fate/flv-add_keyframe_index |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index 445d58d..12bfd4b 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -649,7 +649,7 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream,
>                         sizeof(str_val));
>              av_dict_set(&s->metadata, key, str_val, 0);
>          } else if (amf_type == AMF_DATA_TYPE_NUMBER) {
> -            snprintf(str_val, sizeof(str_val), "%.f", num_val);
> +            snprintf(str_val, sizeof(str_val), "%.12g", num_val);
>              av_dict_set(&s->metadata, key, str_val, 0);
>          } else if (amf_type == AMF_DATA_TYPE_STRING) {
>              av_dict_set(&s->metadata, key, str_val, 0);
> diff --git a/tests/ref/fate/flv-add_keyframe_index b/tests/ref/fate/flv-add_keyframe_index
> index 1c4da65..ce3809e 100644
> --- a/tests/ref/fate/flv-add_keyframe_index
> +++ b/tests/ref/fate/flv-add_keyframe_index
> @@ -7,6 +7,6 @@ canSeekToEnd=true
>  datasize=629776
>  videosize=629381
>  audiosize=0
> -lasttimestamp=20
> +lasttimestamp=19.8571428571
>  lastkeyframetimestamp=19
>  lastkeyframelocation=597963
> --

Ping, any comments?
Michael Niedermayer April 3, 2019, 9:20 p.m. UTC | #2 
On Thu, Mar 28, 2019 at 09:23:29PM +0800, Jun Zhao wrote:
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> Use %.12g replace %.f when save AMF NUMBER(double) type to
> metadata. And update fate ref.
> 
> before this fix, we get FLV metadata like:
> 
>  Metadata:
>     lasttimestamp   : 113
>     lastkeyframetimestamp: 112
> 
> after this fix:
> 
>  Metadata:
>     lasttimestamp   : 113.005
>     lastkeyframetimestamp: 111.678
> 
> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> ---
>  libavformat/flvdec.c                  |    2 +-
>  tests/ref/fate/flv-add_keyframe_index |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index 445d58d..12bfd4b 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -649,7 +649,7 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream,
>                         sizeof(str_val));
>              av_dict_set(&s->metadata, key, str_val, 0);
>          } else if (amf_type == AMF_DATA_TYPE_NUMBER) {
> -            snprintf(str_val, sizeof(str_val), "%.f", num_val);
> +            snprintf(str_val, sizeof(str_val), "%.12g", num_val);
>              av_dict_set(&s->metadata, key, str_val, 0);
>          } else if (amf_type == AMF_DATA_TYPE_STRING) {
>              av_dict_set(&s->metadata, key, str_val, 0);
> diff --git a/tests/ref/fate/flv-add_keyframe_index b/tests/ref/fate/flv-add_keyframe_index
> index 1c4da65..ce3809e 100644
> --- a/tests/ref/fate/flv-add_keyframe_index
> +++ b/tests/ref/fate/flv-add_keyframe_index
> @@ -7,6 +7,6 @@ canSeekToEnd=true
>  datasize=629776
>  videosize=629381
>  audiosize=0
> -lasttimestamp=20
> +lasttimestamp=19.8571428571

does every platform we support produce the same number here ?
if not then make fate would break
its a lot of digits after the decimal point, floats are not exact ...


[...]
Jun Zhao April 4, 2019, 6:42 a.m. UTC | #3 
On Thu, Apr 4, 2019 at 5:21 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Thu, Mar 28, 2019 at 09:23:29PM +0800, Jun Zhao wrote:
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > Use %.12g replace %.f when save AMF NUMBER(double) type to
> > metadata. And update fate ref.
> >
> > before this fix, we get FLV metadata like:
> >
> >  Metadata:
> >     lasttimestamp   : 113
> >     lastkeyframetimestamp: 112
> >
> > after this fix:
> >
> >  Metadata:
> >     lasttimestamp   : 113.005
> >     lastkeyframetimestamp: 111.678
> >
> > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > ---
> >  libavformat/flvdec.c                  |    2 +-
> >  tests/ref/fate/flv-add_keyframe_index |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> > index 445d58d..12bfd4b 100644
> > --- a/libavformat/flvdec.c
> > +++ b/libavformat/flvdec.c
> > @@ -649,7 +649,7 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream,
> >                         sizeof(str_val));
> >              av_dict_set(&s->metadata, key, str_val, 0);
> >          } else if (amf_type == AMF_DATA_TYPE_NUMBER) {
> > -            snprintf(str_val, sizeof(str_val), "%.f", num_val);
> > +            snprintf(str_val, sizeof(str_val), "%.12g", num_val);
> >              av_dict_set(&s->metadata, key, str_val, 0);
> >          } else if (amf_type == AMF_DATA_TYPE_STRING) {
> >              av_dict_set(&s->metadata, key, str_val, 0);
> > diff --git a/tests/ref/fate/flv-add_keyframe_index b/tests/ref/fate/flv-add_keyframe_index
> > index 1c4da65..ce3809e 100644
> > --- a/tests/ref/fate/flv-add_keyframe_index
> > +++ b/tests/ref/fate/flv-add_keyframe_index
> > @@ -7,6 +7,6 @@ canSeekToEnd=true
> >  datasize=629776
> >  videosize=629381
> >  audiosize=0
> > -lasttimestamp=20
> > +lasttimestamp=19.8571428571
>
> does every platform we support produce the same number here ?
> if not then make fate would break
> its a lot of digits after the decimal point, floats are not exact ...

%g flag characters for snprintf is part of C 90 standand, so  I think
it's ok if the platform with C 90 standand suppoted.

For log digits after the decimal point, how about  change from %.12g
to %.6g? Any other suggestion?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iEYEARECAAYFAlylI7AACgkQYR7HhwQLD6uA0gCgk2yOM8TwH8T6oLhoU+20CnRT
XbEAnAuIClxs4buySSU6USaIhAxeqNuV
=tDZ7
-----END PGP SIGNATURE-----
diff mbox

Patch

diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index 445d58d..12bfd4b 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -649,7 +649,7 @@  static int amf_parse_object(AVFormatContext *s, AVStream *astream,
                        sizeof(str_val));
             av_dict_set(&s->metadata, key, str_val, 0);
         } else if (amf_type == AMF_DATA_TYPE_NUMBER) {
-            snprintf(str_val, sizeof(str_val), "%.f", num_val);
+            snprintf(str_val, sizeof(str_val), "%.12g", num_val);
             av_dict_set(&s->metadata, key, str_val, 0);
         } else if (amf_type == AMF_DATA_TYPE_STRING) {
             av_dict_set(&s->metadata, key, str_val, 0);
diff --git a/tests/ref/fate/flv-add_keyframe_index b/tests/ref/fate/flv-add_keyframe_index
index 1c4da65..ce3809e 100644
--- a/tests/ref/fate/flv-add_keyframe_index
+++ b/tests/ref/fate/flv-add_keyframe_index
@@ -7,6 +7,6 @@  canSeekToEnd=true
 datasize=629776
 videosize=629381
 audiosize=0
-lasttimestamp=20
+lasttimestamp=19.8571428571
 lastkeyframetimestamp=19
 lastkeyframelocation=597963