> On Dec 19, 2018, at 4:41 AM, Adrian <adrian-...@o2.pl> wrote: > > Okay, so given you two have some valid points, I've combined both and ended > up with final version of the patch: > - it uses temp file for playlist when either a flag is specified or playlist > type is anything other than VOD > - it does not use temp file for HLS_SINGLE_FILE in place mentioned by Aleksey > > This is my final submission, I believe now everyone should be happy with the > solution. > > Regards > Adrian Guzowski >
Works for me. > W dniu 18.12.2018 o 04:24, Ronak pisze: >>> On Dec 17, 2018, at 4:35 PM, Aleksey Skripka <ca...@undev.ru> wrote: >>> >>> Evening! >>> >>> First of all, about playlist writeout: >>> before 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 - playlist was always(!) >>> creating via .tmp file. >>> after 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 - playlists .tmp logic >>> become dependent on +temp_file flag. >>> >>> I suggest to return to original logic. >>> Non-atomic playlist writeout - is a disaster to just any hls player. This >>> way we will force everyone (except offline VOD) to start using +temp_file. >>> Always! >>> Also, if somebody relying on this - they will get broken streams just after >>> upgrade (It seems to me, that it can be a lot). >>> >>> So, here maintainer should decide how to be. >>> >>> >>> and about single_file: >>> if it will be decided to stay with current way (playlist's .tmp logic >>> controlled by +temp_file flag), you are right - temp_file+single_file case >>> become unreal. >>> if it will be decided to return original behaviour (playlist always via >>> .tmp), someone can choose how to writeout media file (despite i understand >>> temp+single is very-very rare case). >>> >>> Thanks! >>> -- >>> Aleksey Skripk >>> >> Hey Aleksey, >> >> I'm not sure the best thing to do here is to switch to the original logic. >> The temp file is only necessary if you are doing LIVE or EVENT streams; it >> has nothing to do with VOD; where streams are pre-generated. I don't see why >> you would ever want tmp files there. >> Before this commit; ffmpeg used to take more than a week to fragment a 2.2 >> GB Audio File in VOD mode. With this change; it takes less than 1 minute. >> Other competing tools take less than 1 minute. >> >> Also, if you want temp files, you should be supplying the temp_file option - >> the documentation clearly says that: >> https://www.ffmpeg.org/ffmpeg-all.html#hls-2. If you do not want temp files; >> then you should leave this option out. If you are forcing folks to always >> accept the temp file, then why have an option? You should delete the option >> and do it based on the playlist type. >> >> Ronak >> >>>> On 17 Dec 2018, at 19:12, Adrian <adrian-...@o2.pl >>>> <mailto:adrian-...@o2.pl>> wrote: >>>> >>>> Comments inline. >>>> >>>> Regards >>>> Adrian Guzowski >>>> >>>> W dniu 17.12.2018 o 16:56, Aleksey Skripka pisze: >>>>>> On 17 Dec 2018, at 18:45, Adrian <adrian-...@o2.pl >>>>>> <mailto:adrian-...@o2.pl> <mailto:adrian-...@o2.pl >>>>>> <mailto:adrian-...@o2.pl>>> wrote: >>>>>> >>>>>> IMO temp_file+single_file should be mutually exclusive - why would you >>>>>> want to have temp file when you're using only one file anyway? In this >>>>>> mode you would either pool for file changes or just get updated playlist >>>>>> with new byte range (and that one should be updated after video file). I >>>>>> believe this restriction was already in place anyway, because there >>>>>> already were checks for HLS_SINGLE_FILE not being set, before doing >>>>>> actual rename, so this was just a simplification of those conditions. >>>>> not me to decide, please ;) >>>>> if playlist will always be done via .tmp, we will have a choice. >>>> Could you clarify what choice you're talking about? And the purpose of >>>> this choice - frankly you got me lost with this. Regardless, I feel we're >>>> getting off topic. >>>>>> As for playlist file - I'm not sure I follow, I've changed only places >>>>>> where temp files were already used, so I don't think I broke something >>>>>> in that matter. >>>>> not you, it was introduced by 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 >>>> Not quite, temp files were already a feature, this commit narrowed >>>> conditions where they are used. If you're referring to change in >>>> hls_window about not checking TEMP flag - that is unrelated to current >>>> state (before commit introducing regression, check was ignoring TEMP flag, >>>> after commit it does take it into account - probably should be discussed >>>> with person responsible for original change as to why it was changed that >>>> way, so that's out of scope of this patch. Issue at hand is not using TEMP >>>> files at all and that is fixed by this patch. >>>> >>>>>> From repeat+level+trace I believe I saw that playlist file actually was >>>>>> using a temp file, so if that's the thing you are concerned about, it >>>>>> seems like it's all good. >>>>>> >>>>>> Regards >>>>>> Adrian Guzowski >>>>>> >>>>>> W dniu 17.12.2018 o 16:36, Aleksey Skripka pisze: >>>>>>> Greetings! >>>>>>> >>>>>>> Adrian, >>>>>>> with your variant of patch in case of '-hls_flags >>>>>>> +temp_file+single_file' no .tmp logic will be used. >>>>>>> not sure what is better, but all previous versions was doing so. >>>>>>> >>>>>>> and also, i'm still sure, that for backward compatibility with all >>>>>>> previous versions, index.m3u8 should always be written via .tmp file. >>>>>>> >>>>>>> ps: >>>>>>> i noticed this bug such way: i do not use +temp_file, but rely on >>>>>>> atomic rename of index.m3u8. >>>>>>> >>>>>>>> On 17 Dec 2018, at 17:23, Adrian <adrian-...@o2.pl> wrote: >>>>>>>> >>>>>>>> Hello, >>>>>>>> >>>>>>>> after upgrading FFmpeg from 4.0 to 4.1 I noticed that temp files in >>>>>>>> HLS muxed stopped working. >>>>>>>> It looks like a regression introduced by >>>>>>>> 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6. I've prepared a patch and >>>>>>>> tested it cross-compiling for my project's target platform (ARM, >>>>>>>> Buildroot) and it seems like everything is in order. I ran regression >>>>>>>> tests and nothing seems to be broken. >>>>>>>> >>>>>>>> Please review it and possibly include this patch. >>>>>>>> >>>>>>>> Regards >>>>>>>> Adrian Guzowski >>>>>>>> >>>>>>>> <0001-Fix-usage-of-temp_file-flag-in-hls_flags-option.patch>_______________________________________________ >>>>>>>> ffmpeg-devel mailing list >>>>>>>> ffmpeg-devel@ffmpeg.org >>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>>>> _______________________________________________ >>>>>> ffmpeg-devel mailing list >>>>>> ffmpeg-devel@ffmpeg.org >>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>>> _______________________________________________ >>>>> ffmpeg-devel mailing list >>>>> ffmpeg-devel@ffmpeg.org >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>> _______________________________________________ >>>> ffmpeg-devel mailing list >>>> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> >>>> <mailto:ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>> >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>> <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel> >>>> <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>> <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>> >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > <0001-Fix-usage-of-temp_file-flag-in-hls_flags-option-v3.patch>_______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
