On 03/09/14 12:55, "Martin Storsjö" <[email protected]> wrote:
>On Wed, 3 Sep 2014, Mika Raento wrote:
>
>> This is a non-standard file that maps the MSS segment names to offsets
>> in the ISMV file. This can be used to build a custom MSS streaming
>> server without splitting the ISMV into separate files.
>> ---
>> tools/ismindex.c | 81
>>+++++++++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 68 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/ismindex.c b/tools/ismindex.c
>> index a6a9763..47846bd 100644
>> --- a/tools/ismindex.c
>> +++ b/tools/ismindex.c
>> @@ -23,7 +23,9 @@
>> * avconv <normal input/transcoding options> -movflags frag_keyframe
>>foo.ismv
>> * ismindex -n foo foo.ismv
>> * This step creates foo.ism and foo.ismc that is required by IIS for
>> - * serving it.
>> + * serving it. It also creates foo.ismf, which maps fragment names to
>> + * start-end offsets in the ismv, for use in your own streaming server
>> + * (the .ismf file is not created if -split is given).
>> *
>
>I'd rather have a separate option for doing this, instead of always
>producing a non-standard file that most people won't use. Like -ismf or
>so?
>
>> * By adding -path-prefix path/, the produced foo.ism will refer to the
>> * files foo.ismv as "path/foo.ismv" - the prefix for the generated ismc
>> @@ -118,6 +120,21 @@ static int copy_tag(AVIOContext *in, AVIOContext
>>*out, int32_t tag_name)
>> return 0;
>> }
>>
>> +static int skip_tag(AVIOContext *in, int32_t tag_name)
>> +{
>> + int64_t pos = avio_tell(in);
>
>This has got some unnecessary spaces before the = - you probably copied
>it
>from somewhere where it was vertically aligned with something above/below.
>
>> + int32_t size, tag;
>> +
>> + size = avio_rb32(in);
>> + tag = avio_rb32(in);
>> + if (tag != tag_name) {
>> + fprintf(stderr, "wanted tag %d, got %d", tag_name, tag);
>
>Missing \n, and this could perhaps also write the literal tags similarly
>to copy_tag (perhaps that part could be split to a separate function,
>like
>expect_tag, which does the check and prints an error)
>
>> + return -1;
>> + }
>> + avio_seek(in, pos + size, SEEK_SET);
>> + return 0;
>> +}
>> +
>> static int write_fragment(const char *filename, AVIOContext *in)
>> {
>> AVIOContext *out = NULL;
>> @@ -139,25 +156,62 @@ static int write_fragment(const char *filename,
>>AVIOContext *in)
>> return ret;
>> }
>>
>> +static int skip_fragment(AVIOContext *in)
>> +{
>> + return (skip_tag(in, MKBETAG('m', 'o', 'o', 'f')) ||
>> + skip_tag(in, MKBETAG('m', 'd', 'a', 't')));
>> +}
>
>As you noted yourself, this could be written slightly differently
>
>> +
>> static int write_fragments(struct Tracks *tracks, int start_index,
>> - AVIOContext *in, const char *output_prefix)
>> + AVIOContext *in, const char *basename,
>> + int split, const char* output_prefix)
>> {
>> - char dirname[2048], filename[2048];
>> - int i, j;
>> + char dirname[2048], filename[2048], idxname[2048];
>> + int i, j, ret, fragment_ret;
>> + FILE* out;
>>
>> + out = NULL;
>> + ret = 0;
>> + if (!split) {
>> + snprintf(idxname, sizeof(idxname), "%s%s.ismf", output_prefix,
>>basename);
>> + out = fopen(idxname, "w");
>> + if (!out) {
>> + ret = AVERROR(errno);
>> + perror(idxname);
>> + goto fail;
>> + }
>> + }
>> for (i = start_index; i < tracks->nb_tracks; i++) {
>> struct Track *track = tracks->tracks[i];
>> const char *type = track->is_video ? "video" : "audio";
>> snprintf(dirname, sizeof(dirname), "%sQualityLevels(%d)",
>>output_prefix, track->bitrate);
>> - mkdir(dirname, 0777);
>> + if (split) {
>> + if (mkdir(dirname, 0777) == -1) {
>> + ret = AVERROR(errno);
>> + perror(dirname);
>> + goto fail;
>> + }
>> + }
>> for (j = 0; j < track->chunks; j++) {
>> snprintf(filename, sizeof(filename),
>>"%s/Fragments(%s=%"PRId64")",
>> dirname, type, track->offsets[j].time);
>> avio_seek(in, track->offsets[j].offset, SEEK_SET);
>> - write_fragment(filename, in);
>> + if (split) {
>> + fragment_ret = write_fragment(filename, in);
>> + } else {
>> + fprintf(out, "%s %"PRId64, filename, avio_tell(in));
>> + fragment_ret = skip_fragment(in);
>> + fprintf(out, " %"PRId64"\n", avio_tell(in));
>> + }
>> + if (fragment_ret != 0) {
>> + fprintf(stderr, "failed fragment");
>
>Missing \n
>
>> + ret = fragment_ret;
>> + }
>> }
>> }
>> - return 0;
>> +fail:
>> + if (out) fclose(out);
>
>This is mostly a nitpick, but within the libav codebase, we mostly write
>conditions like this with the fclose() on a separate line (unless it
>significantly impairs readability, for some table-like things).
>
>
>The rest of it looks quite good though!
>
>// Martin
>
Thanks for the comments, all fine by me, will resubmit in a bit.
Mika
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
