Hi,
On Mon, 1 Sep 2014, Mika Raento wrote:
This improves error messages for completely and somewhat broken inputs.
---
tools/ismindex.c | 49 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 37 insertions(+), 12 deletions(-)
diff --git a/tools/ismindex.c b/tools/ismindex.c
index a2eed87..4eba910 100644
--- a/tools/ismindex.c
+++ b/tools/ismindex.c
@@ -96,14 +96,19 @@ static int copy_tag(AVIOContext *in, AVIOContext *out,
int32_t tag_name)
tag = avio_rb32(in);
avio_wb32(out, size);
avio_wb32(out, tag);
- if (tag != tag_name)
+ if (tag != tag_name) {
+ fprintf(stderr, "wanted tag %d, got %d", tag_name, tag);
return -1;
+ }
This could use %.4s instead of %d to make it even more readable. But to
do that properly we'd need to write the tag to a string buffer in the
original endianness.
The message is also missing a \n
size -= 8;
while (size > 0) {
char buf[1024];
int len = FFMIN(sizeof(buf), size);
- if (avio_read(in, buf, len) != len)
+ int got;
+ if ((got =avio_read(in, buf, len)) != len) {
Space after =
+ fprintf(stderr, "short read, wanted %d, got %d", len, got);
Missing \n
break;
+ }
avio_write(out, buf, len);
size -= len;
}
@@ -115,10 +120,12 @@ static int write_fragment(const char *filename,
AVIOContext *in)
AVIOContext *out = NULL;
int ret;
- if ((ret = avio_open2(&out, filename, AVIO_FLAG_WRITE, NULL, NULL)) < 0)
+ if ((ret = avio_open2(&out, filename, AVIO_FLAG_WRITE, NULL, NULL)) < 0) {
+ perror(filename);
avio_open2 doesn't necessarily set errno (if you use a plain file, it
might set errno, but if using some other protocol it probably won't) -
actually using the return code requires using av_strerror.
return ret;
- copy_tag(in, out, MKBETAG('m', 'o', 'o', 'f'));
- copy_tag(in, out, MKBETAG('m', 'd', 'a', 't'));
+ }
+ ret = (copy_tag(in, out, MKBETAG('m', 'o', 'o', 'f')) ||
+ copy_tag(in, out, MKBETAG('m', 'd', 'a', 't')));
This idiom isn't used all that much within libav (I think - I find it a
bit hard to read). What about something like this?
ret = copy_tag(in, out, MKBETAG('m', 'o', 'o', 'f'));
if (!ret)
ret = copy_tag(in, out, MKBETAG('m', 'd', 'a', 't'));
avio_flush(out);
avio_close(out);
@@ -207,6 +214,7 @@ static int read_mfra(struct Tracks *tracks, int start_index,
const char *file, int split, const char *output_prefix)
{
int err = 0;
+ const char* err_str = "";
AVIOContext *f = NULL;
int32_t mfra_size;
@@ -217,10 +225,12 @@ static int read_mfra(struct Tracks *tracks, int
start_index,
avio_seek(f, -mfra_size, SEEK_CUR);
if (avio_rb32(f) != mfra_size) {
err = AVERROR_INVALIDDATA;
+ err_str = "mfra size mismatch";
goto fail;
}
if (avio_rb32(f) != MKBETAG('m', 'f', 'r', 'a')) {
err = AVERROR_INVALIDDATA;
+ err_str = "mfra tag mismatch";
goto fail;
}
while (!read_tfra(tracks, start_index, f)) {
@@ -234,7 +244,7 @@ fail:
if (f)
avio_close(f);
if (err)
- fprintf(stderr, "Unable to read the MFRA atom in %s\n", file);
+ fprintf(stderr, "Unable to read the MFRA atom in %s (%s)\n", file,
err_str);
return err;
}
@@ -257,7 +267,10 @@ static int get_video_private_data(struct Track *track,
AVCodecContext *codec)
if (codec->codec_id == AV_CODEC_ID_VC1)
return get_private_data(track, codec);
- avio_open_dyn_buf(&io);
+ if (avio_open_dyn_buf(&io) < 0) {
+ err = AVERROR(ENOMEM);
+ goto fail;
+ }
Even though ENOMEM is the most probable error here, it should rather
actually store the return value. (Not that it really matters here, but
it's easy to do and good practice.) Given how this function uses a default
set EINVAL it does require a little more changes though...
if (codec->extradata_size < 11 || codec->extradata[0] != 1)
goto fail;
sps_size = AV_RB16(&codec->extradata[6]);
@@ -431,12 +444,24 @@ static void print_track_chunks(FILE *out, struct Tracks
*tracks, int main,
{
int i, j;
struct Track *track = tracks->tracks[main];
+ int should_print_time_mismatch = 1;
+
for (i = 0; i < track->chunks; i++) {
for (j = main + 1; j < tracks->nb_tracks; j++) {
if (tracks->tracks[j]->is_audio == track->is_audio &&
- track->offsets[i].duration !=
tracks->tracks[j]->offsets[i].duration)
- fprintf(stderr, "Mismatched duration of %s chunk %d in %s and
%s\n",
- type, i, track->name, tracks->tracks[j]->name);
+ track->offsets[i].duration !=
tracks->tracks[j]->offsets[i].duration) {
+ fprintf(stderr, "Mismatched duration of %s chunk %d in %s (%d) and
%s (%d)\n",
+ type, i, track->name, main, tracks->tracks[j]->name,
j);
+ should_print_time_mismatch = 1;
+ }
+ if (tracks->tracks[j]->is_audio == track->is_audio &&
+ track->offsets[i].time != tracks->tracks[j]->offsets[i].time) {
+ if (should_print_time_mismatch) {
+ fprintf(stderr, "Mismatched (start) time of %s chunk %d in %s
(%d) and %s (%d)\n",
+ type, i, track->name, main,
tracks->tracks[j]->name, j);
+ }
+ should_print_time_mismatch = 0;
+ }
}
I think I'd rather group both conditions under one single if
(...->is_audio == track->is_audio), to make it slightly more readable.
All in all it looks good, I'll post an amended version of your patch with
my remarks corrected - if it's ok with you I'll push it tomorrow or so.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
