10.07.2014 02:40, Nicolas Martyanoff kirjoitti: > On 2014-07-10 01:32, Anssi Hannula wrote: >> avformat/hlsenc: cleaning >> - looks good to me, and there is no maintainer so I guess this is OK >> - well, actually, maybe you could use more than one word in the subject :) > I'll change the title to something such as 'make the code easier to read' :) > >> avformat/hlsenc: add single file mode >> - Adds HLSContext.ref_stream etc, shouldn't that be a separate patch? > I guess I could, but the code is incorrect without it. I added it when I > realized it would not work when a file has two video streams (or two audio > streams and no video streams). > > If you think it's better to have two patches, the first one being incorrect, I > can split it.
Maybe I'm missing something, but I don't see how not working with two video streams has anything to do with single file mode? Assuming there is actually some relation, could you have the ref changes in a first patch and single file in another patch, or would the ref changes break something else? >> - HLSContext.file_idx is never read AFAICS > It is used to generate the TS filename in hls_create_file(). I don't see hls->file_idx accessed there... only the local int file_idx. >> avformat/hslenc: add a flag disabling the filename in segment names >> - hsl > Oops >> - should probably require single_file as well? > Indeed, I'll fix it. > >> - see below >> >> avformat/hlsenc: add an option to set the media filename >> - should probably check not used together with separated segments? > It also works with separated segments. For example: > > -hls_media_filename foo-%d.ts > > Will generate foo-0.ts, foo-1.ts, foo-2.ts, etc. > > Without this patch, separated files are named <playlist_base_filename>%d.ts. Right. This should be documented in doc/muxers.texi, though. Same goes for your other new parameters, BTW, I forgot about that. >> - see below >> >> >> "no_filename" for "do not use the name of the media file in segment >> names" were unclear enough that I had to take a look at the code what is >> going on. >> >> For anyone else wondering (correct me if I'm wrong), currently: >> (1) The output file name from the user is used as the output media >> playlist filename. >> (2) The media file/segment file names are generated from the basename of >> the output playlist filename. >> (3) The media file/segments URLs in the media playlist get -hls_base_url >> prepended to them, so they are base_url + media filename. > This is indeed how it worked before my patches. > >> These two patches would >> - add "no_filename" flag to only use hls_base_url for (3), allowing to >> select an arbitrary URL. >> - allow to select the file for (2) > Correct. > >> I wonder if it would be clearer to have >> -hls_media_file - "the generated output media file" >> (requires "single_file") > Not that as explained before, -hls_media_file is valid and used for > !single_file (i.e. individual files). > >> -hls_media_url - "output media file url used in the playlist" >> (requires "single_file" and !hls_base_url) >> >> Or would it just be unclear to have both conflicting hls_media_url and >> hls_base_url? > I do not know. On the one hand, it seems redundant to have two different > options to specify the same thing, i.e. the URL used for segments. On the > other hand, with (HLS_SINGLE_FILE | HLS_NO_FILENAME), there is no "base URL", > only a "normal" URL. > > It would be useful to have the point of view of someone using this muxer. > > > Thank you for taking the time to review my patches ! > > I'll update my patches tomorrow. > > Regards, > -- Anssi Hannula _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
