[2/8] mov: Print reason of loci parsing failure
diff mbox

Message ID 1463569341-73691-2-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö May 18, 2016, 11:02 a.m. 
From: Michael Niedermayer <michaelni@gmx.at>

---
 libavformat/mov.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Diego Biurrun May 18, 2016, 2:23 p.m. | #1 
On Wed, May 18, 2016 at 02:02:15PM +0300, Martin Storsjö wrote:
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -235,13 +237,17 @@ static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
> -    if (len < 14)
> +    if (len < 14) {
> +        av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);

len is unsigned.

The message is a tad confusing.  I'm not sure why you print len at all.
Something along the lines of "x required, y available" would make sense
to me, but just printing the available size w/o further context does not
help I believe.

Diego
Martin Storsjö May 18, 2016, 7:09 p.m. | #2 
On Wed, 18 May 2016, Diego Biurrun wrote:

> On Wed, May 18, 2016 at 02:02:15PM +0300, Martin Storsjö wrote:
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -235,13 +237,17 @@ static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
>> -    if (len < 14)
>> +    if (len < 14) {
>> +        av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);
>
> len is unsigned.
>
> The message is a tad confusing.  I'm not sure why you print len at all.
> Something along the lines of "x required, y available" would make sense
> to me, but just printing the available size w/o further context does not
> help I believe.

Well, it does help for the one who debugs it at least, but printing more 
shouldn't hurt of course.

Changed locally into this:

     av_log(c->fc, AV_LOG_ERROR,
            "loci too short (%u bytes left, need at least %d)\n", len, 14);

Ok with that changed?

// Martin
Luca Barbato May 18, 2016, 7:54 p.m. | #3 
On 18/05/16 21:09, Martin Storsjö wrote:
> On Wed, 18 May 2016, Diego Biurrun wrote:
> 
>> On Wed, May 18, 2016 at 02:02:15PM +0300, Martin Storsjö wrote:
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -235,13 +237,17 @@ static int mov_metadata_loci(MOVContext *c,
>>> AVIOContext *pb, unsigned len)
>>> -    if (len < 14)
>>> +    if (len < 14) {
>>> +        av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left
>>> (%d)\n", len);
>>
>> len is unsigned.
>>
>> The message is a tad confusing.  I'm not sure why you print len at all.
>> Something along the lines of "x required, y available" would make sense
>> to me, but just printing the available size w/o further context does not
>> help I believe.
> 
> Well, it does help for the one who debugs it at least, but printing more
> shouldn't hurt of course.
> 
> Changed locally into this:
> 
>     av_log(c->fc, AV_LOG_ERROR,
>            "loci too short (%u bytes left, need at least %d)\n", len, 14);
> 
> Ok with that changed?

I like it.
Diego Biurrun May 19, 2016, 7:35 a.m. | #4 
On Wed, May 18, 2016 at 10:09:06PM +0300, Martin Storsjö wrote:
> On Wed, 18 May 2016, Diego Biurrun wrote:
> 
> > On Wed, May 18, 2016 at 02:02:15PM +0300, Martin Storsjö wrote:
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -235,13 +237,17 @@ static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
> >> -    if (len < 14)
> >> +    if (len < 14) {
> >> +        av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);
> >
> > len is unsigned.
> >
> > The message is a tad confusing.  I'm not sure why you print len at all.
> > Something along the lines of "x required, y available" would make sense
> > to me, but just printing the available size w/o further context does not
> > help I believe.
> 
> Well, it does help for the one who debugs it at least, but printing more 
> shouldn't hurt of course.
> 
> Changed locally into this:
> 
>      av_log(c->fc, AV_LOG_ERROR,
>             "loci too short (%u bytes left, need at least %d)\n", len, 14);
> 
> Ok with that changed?

Yes.

Diego

Patch
diff mbox 

diff --git a/libavformat/mov.c b/libavformat/mov.c
index b4ff4ff..42a5220 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -226,8 +226,10 @@  static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
     double longitude, latitude;
     const char *key = "location";
 
-    if (len < 4 + 2 + 1 + 1 + 4 + 4 + 4)
+    if (len < 4 + 2 + 1 + 1 + 4 + 4 + 4) {
+        av_log(c->fc, AV_LOG_ERROR, "loci too short\n");
         return AVERROR_INVALIDDATA;
+    }
 
     avio_skip(pb, 4); // version+flags
     langcode = avio_rb16(pb);
@@ -235,13 +237,17 @@  static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
     len -= 6;
 
     len -= avio_get_str(pb, len, buf, sizeof(buf)); // place name
-    if (len < 1)
+    if (len < 1) {
+        av_log(c->fc, AV_LOG_ERROR, "place name too long\n");
         return AVERROR_INVALIDDATA;
+    }
     avio_skip(pb, 1); // role
     len -= 1;
 
-    if (len < 14)
+    if (len < 14) {
+        av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);
         return AVERROR_INVALIDDATA;
+    }
     longitude = ((int32_t) avio_rb32(pb)) / (float) (1 << 16);
     latitude  = ((int32_t) avio_rb32(pb)) / (float) (1 << 16);