diff mbox

[FFmpeg-devel,V1] lavf/oggparsevorbis: Fix change the case of metadata keys issue

Message ID 1555293510-7402-1-git-send-email-mypopydev@gmail.com
State Superseded
Headers show

Commit Message

Jun Zhao April 15, 2019, 1:58 a.m. UTC 
From: Jun Zhao <barryjzhao@tencent.com>

The spec in https://xiph.org/vorbis/doc/v-comment.html states that
the metadata keys are case-insensitive, so don't change the case
and update the fate test case.

Fix #7784

Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 libavformat/oggparsevorbis.c             |    9 ++++-----
 tests/ref/fate/limited_input_seek        |    2 +-
 tests/ref/fate/limited_input_seek-copyts |    2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer April 15, 2019, 9:51 p.m. UTC | #1 
On Mon, Apr 15, 2019 at 09:58:30AM +0800, Jun Zhao wrote:
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> The spec in https://xiph.org/vorbis/doc/v-comment.html states that
> the metadata keys are case-insensitive, so don't change the case
> and update the fate test case.
> 
> Fix #7784
> 
> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> ---
>  libavformat/oggparsevorbis.c             |    9 ++++-----
>  tests/ref/fate/limited_input_seek        |    2 +-
>  tests/ref/fate/limited_input_seek-copyts |    2 +-
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index bcfd246..c3c8d38 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -44,7 +44,7 @@ static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
>      int i, cnum, h, m, s, ms, keylen = strlen(key);
>      AVChapter *chapter = NULL;
>  
> -    if (keylen < 9 || sscanf(key, "CHAPTER%03d", &cnum) != 1)
> +    if (keylen < 9 || (av_strcasecmp(key, "CHAPTER")!=0 && sscanf(key+7, "%03d", &cnum) != 1))

this looks a bit odd, shouldnt this use av_strncasecmp() ?


[...]
mypopy@gmail.com April 16, 2019, 1:33 a.m. UTC | #2 
On Tue, Apr 16, 2019 at 5:51 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, Apr 15, 2019 at 09:58:30AM +0800, Jun Zhao wrote:
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > The spec in https://xiph.org/vorbis/doc/v-comment.html states that
> > the metadata keys are case-insensitive, so don't change the case
> > and update the fate test case.
> >
> > Fix #7784
> >
> > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > ---
> >  libavformat/oggparsevorbis.c             |    9 ++++-----
> >  tests/ref/fate/limited_input_seek        |    2 +-
> >  tests/ref/fate/limited_input_seek-copyts |    2 +-
> >  3 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > index bcfd246..c3c8d38 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -44,7 +44,7 @@ static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
> >      int i, cnum, h, m, s, ms, keylen = strlen(key);
> >      AVChapter *chapter = NULL;
> >
> > -    if (keylen < 9 || sscanf(key, "CHAPTER%03d", &cnum) != 1)
> > +    if (keylen < 9 || (av_strcasecmp(key, "CHAPTER")!=0 && sscanf(key+7, "%03d", &cnum) != 1))
>
> this looks a bit odd, shouldnt this use av_strncasecmp() ?
>
It's  in order to be compatible with the fate test case
vorbis-1833-chapthers, in this test,
we use lower case in test clip but check upper case in the code.
Michael Niedermayer April 16, 2019, 10:41 p.m. UTC | #3 
On Tue, Apr 16, 2019 at 09:33:08AM +0800, mypopy@gmail.com wrote:
> On Tue, Apr 16, 2019 at 5:51 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Mon, Apr 15, 2019 at 09:58:30AM +0800, Jun Zhao wrote:
> > > From: Jun Zhao <barryjzhao@tencent.com>
> > >
> > > The spec in https://xiph.org/vorbis/doc/v-comment.html states that
> > > the metadata keys are case-insensitive, so don't change the case
> > > and update the fate test case.
> > >
> > > Fix #7784
> > >
> > > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > > ---
> > >  libavformat/oggparsevorbis.c             |    9 ++++-----
> > >  tests/ref/fate/limited_input_seek        |    2 +-
> > >  tests/ref/fate/limited_input_seek-copyts |    2 +-
> > >  3 files changed, 6 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > > index bcfd246..c3c8d38 100644
> > > --- a/libavformat/oggparsevorbis.c
> > > +++ b/libavformat/oggparsevorbis.c
> > > @@ -44,7 +44,7 @@ static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
> > >      int i, cnum, h, m, s, ms, keylen = strlen(key);
> > >      AVChapter *chapter = NULL;
> > >
> > > -    if (keylen < 9 || sscanf(key, "CHAPTER%03d", &cnum) != 1)
> > > +    if (keylen < 9 || (av_strcasecmp(key, "CHAPTER")!=0 && sscanf(key+7, "%03d", &cnum) != 1))
> >
> > this looks a bit odd, shouldnt this use av_strncasecmp() ?
> >
> It's  in order to be compatible with the fate test case
> vorbis-1833-chapthers, in this test,
> we use lower case in test clip but check upper case in the code.

av_strcasecmp() will only succeed if the string ends after "CHAPTER" and
contains no number which is why av_strncasecmp() feels like more effective.
The problem is ultimatle that keylen is at this point >= 9 while
the only case where av_strcasecmp with "CHAPTER" == 0 is with a
keylen of 7. Thats tricky to achieve ;)

Or said differently this code is not a case insensitive version of the
previous 


[...]
mypopy@gmail.com April 17, 2019, 2:04 a.m. UTC | #4 
On Wed, Apr 17, 2019 at 6:41 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Tue, Apr 16, 2019 at 09:33:08AM +0800, mypopy@gmail.com wrote:
> > On Tue, Apr 16, 2019 at 5:51 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Mon, Apr 15, 2019 at 09:58:30AM +0800, Jun Zhao wrote:
> > > > From: Jun Zhao <barryjzhao@tencent.com>
> > > >
> > > > The spec in https://xiph.org/vorbis/doc/v-comment.html states that
> > > > the metadata keys are case-insensitive, so don't change the case
> > > > and update the fate test case.
> > > >
> > > > Fix #7784
> > > >
> > > > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > > > ---
> > > >  libavformat/oggparsevorbis.c             |    9 ++++-----
> > > >  tests/ref/fate/limited_input_seek        |    2 +-
> > > >  tests/ref/fate/limited_input_seek-copyts |    2 +-
> > > >  3 files changed, 6 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > > > index bcfd246..c3c8d38 100644
> > > > --- a/libavformat/oggparsevorbis.c
> > > > +++ b/libavformat/oggparsevorbis.c
> > > > @@ -44,7 +44,7 @@ static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
> > > >      int i, cnum, h, m, s, ms, keylen = strlen(key);
> > > >      AVChapter *chapter = NULL;
> > > >
> > > > -    if (keylen < 9 || sscanf(key, "CHAPTER%03d", &cnum) != 1)
> > > > +    if (keylen < 9 || (av_strcasecmp(key, "CHAPTER")!=0 && sscanf(key+7, "%03d", &cnum) != 1))
> > >
> > > this looks a bit odd, shouldnt this use av_strncasecmp() ?
> > >
> > It's  in order to be compatible with the fate test case
> > vorbis-1833-chapthers, in this test,
> > we use lower case in test clip but check upper case in the code.
>
> av_strcasecmp() will only succeed if the string ends after "CHAPTER" and
> contains no number which is why av_strncasecmp() feels like more effective.
> The problem is ultimatle that keylen is at this point >= 9 while
> the only case where av_strcasecmp with "CHAPTER" == 0 is with a
> keylen of 7. Thats tricky to achieve ;)
>
> Or said differently this code is not a case insensitive version of the
> previous
Thanks the comments, will update the patch
diff mbox

Patch

diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index bcfd246..c3c8d38 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -44,7 +44,7 @@  static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
     int i, cnum, h, m, s, ms, keylen = strlen(key);
     AVChapter *chapter = NULL;
 
-    if (keylen < 9 || sscanf(key, "CHAPTER%03d", &cnum) != 1)
+    if (keylen < 9 || (av_strcasecmp(key, "CHAPTER")!=0 && sscanf(key+7, "%03d", &cnum) != 1))
         return 0;
 
     if (keylen <= 10) {
@@ -55,7 +55,7 @@  static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
                            ms + 1000 * (s + 60 * (m + 60 * h)),
                            AV_NOPTS_VALUE, NULL);
         av_free(val);
-    } else if (!strcmp(key + keylen - 4, "NAME")) {
+    } else if (!av_strcasecmp(key + keylen - 4, "NAME")) {
         for (i = 0; i < as->nb_chapters; i++)
             if (as->chapters[i]->id == cnum) {
                 chapter = as->chapters[i];
@@ -91,7 +91,7 @@  int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
     const uint8_t *p   = buf;
     const uint8_t *end = buf + size;
     int updates        = 0;
-    unsigned n, j;
+    unsigned n;
     int s;
 
     /* must have vendor_length and user_comment_list_length */
@@ -139,8 +139,7 @@  int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
                 return AVERROR(ENOMEM);
             }
 
-            for (j = 0; j < tl; j++)
-                tt[j] = av_toupper(t[j]);
+            memcpy(tt, t, tl);
             tt[tl] = 0;
 
             memcpy(ct, v, vl);
diff --git a/tests/ref/fate/limited_input_seek b/tests/ref/fate/limited_input_seek
index e0c4bf1..3269dce 100644
--- a/tests/ref/fate/limited_input_seek
+++ b/tests/ref/fate/limited_input_seek
@@ -1 +1 @@ 
-20a1bb9a1cfb23c1fe86f14e6065cd95
+ae878bdaec23f36a63d142165fe57f49
diff --git a/tests/ref/fate/limited_input_seek-copyts b/tests/ref/fate/limited_input_seek-copyts
index 92790a8..6eeaef8 100644
--- a/tests/ref/fate/limited_input_seek-copyts
+++ b/tests/ref/fate/limited_input_seek-copyts
@@ -1 +1 @@ 
-ec3604b1954ed80de364b8ef491771ce
+ffe8a674bdf38e4f650f91963debc654