[FFmpeg-devel] avformat/hlsenc: pull request for single file mode

[Anssi Hannula]

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