On 04/09/14 14:10, "Martin Storsjö" <[email protected]> wrote:
>On Thu, 4 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 | 115
>>+++++++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 95 insertions(+), 20 deletions(-)
>
>Ok, now this is quite close. Sorry that I have a few more comments that I
>didn't notice earlier...
>
>> diff --git a/tools/ismindex.c b/tools/ismindex.c
>> index a6a9763..85b520f 100644
>> --- a/tools/ismindex.c
>> +++ b/tools/ismindex.c
>> +
>> static int write_fragments(struct Tracks *tracks, int start_index,
>> - AVIOContext *in, const char *output_prefix)
>> + AVIOContext *in, const char *basename,
>> + int split, int ismf, 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;
>
>These initializations can be merged with the declaration, saving a few
>lines
>
>> + if (ismf) {
>> + 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;
>> + }
>> + }
>
>Here I actually intentionally ignored errors from mkdir before.
>Especially
>when testing these tools, it's quite handy to be able to overwrite files
>in place, but mkdir fails if the directory already exists. So here (and
>similarly in libavformat/smoothstreamingenc.c), if mkdir fails we just
>ignore it - we need to check if creating the files worked anyway.
>
>(The downside is that if creating the directory failed, the error
>messages
>are slightly more confusing. The ideal solution would be to use stat or
>something similar to check whether the directory already exists, but then
>we end up with more function calls that might not be fully portable etc,
>so I've kept it simple.)
>
>> 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 (ismf) {
>> + fprintf(out, "%s %"PRId64, filename, avio_tell(in));
>> + }
>> + if (split) {
>> + fragment_ret = write_fragment(filename, in);
>> + } else {
>> + fragment_ret = skip_fragment(in);
>> + }
>> + if (ismf) {
>> + fprintf(out, " %"PRId64"\n", avio_tell(in));
>> + }
>
>I think this would be more readable without the extra braces for the
>single-line conditions - at least it would keep the number of lines
>down...
>
>> + if (fragment_ret != 0) {
>> + fprintf(stderr, "failed fragment %d\n", j);
>> + ret = fragment_ret;
>> + }
>
>Technically this is also a behaviour change from before - previously
>these
>errors were ignored. But this was ignored more due to lazyness than
>intentionally, so I guess it's ok to add it here.
>
>> }
>> }
>> - return 0;
>> +fail:
>> + if (out)
>> + fclose(out);
>> + return ret;
>> }
>>
>> static int read_tfra(struct Tracks *tracks, int start_index,
>>AVIOContext *f)
>> @@ -217,7 +285,8 @@ fail:
>> }
>>
>> static int read_mfra(struct Tracks *tracks, int start_index,
>> - const char *file, int split, const char
>>*output_prefix)
>> + const char *file, int split, int ismf,
>> + const char *basename, const char* output_prefix)
>> {
>> int err = 0;
>> const char* err_str = "";
>> @@ -243,8 +312,9 @@ static int read_mfra(struct Tracks *tracks, int
>>start_index,
>> /* Empty */
>> }
>>
>> - if (split)
>> - write_fragments(tracks, start_index, f, output_prefix);
>> + err = write_fragments(tracks, start_index, f, basename, split,
>>ismf,
>> + output_prefix);
>> + err_str = "error in write_fragments";
>
>Wouldn't it make more sense to keep the condition, but expand it into "if
>(split || ismf)"?
>
>// Martin
>
Good comments, will address and re-submit. -mika
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
