| Message ID | 54cc632b-8a26-291f-0334-849a410c7ac1@o2.pl |
|---|---|
| State | New |
| Headers | show |
> On Dec 19, 2018, at 4:41 AM, Adrian <adrian-007@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 <caspy@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-007@o2.pl <mailto:adrian-007@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-007@o2.pl <mailto:adrian-007@o2.pl> <mailto:adrian-007@o2.pl <mailto:adrian-007@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-007@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
All my tests ok. Thank you, Adrian. > On 19 Dec 2018, at 12:41, Adrian <adrian-007@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 > > W dniu 18.12.2018 o 04:24, Ronak pisze: >>> On Dec 17, 2018, at 4:35 PM, Aleksey Skripka <caspy@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-007@o2.pl <mailto:adrian-007@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-007@o2.pl <mailto:adrian-007@o2.pl> <mailto:adrian-007@o2.pl <mailto:adrian-007@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-007@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
> 在 2018年12月20日，上午12:19，Aleksey Skripka <caspy@undev.ru> 写道： > > All my tests ok. > Thank you, Adrian. > >> On 19 Dec 2018, at 12:41, Adrian <adrian-007@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 >> >> W dniu 18.12.2018 o 04:24, Ronak pisze: >>>> On Dec 17, 2018, at 4:35 PM, Aleksey Skripka <caspy@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-007@o2.pl <mailto:adrian-007@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-007@o2.pl <mailto:adrian-007@o2.pl> <mailto:adrian-007@o2.pl <mailto:adrian-007@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-007@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 will push this patch after 48hours if there have no objtections. Thanks Steven
> 在 2018年12月20日，上午12:19，Aleksey Skripka <caspy@undev.ru> 写道： > > All my tests ok. > Thank you, Adrian. > >> On 19 Dec 2018, at 12:41, Adrian <adrian-007@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 >> >> W dniu 18.12.2018 o 04:24, Ronak pisze: >>>> On Dec 17, 2018, at 4:35 PM, Aleksey Skripka <caspy@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-007@o2.pl <mailto:adrian-007@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-007@o2.pl <mailto:adrian-007@o2.pl> <mailto:adrian-007@o2.pl <mailto:adrian-007@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-007@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>_______________________________________________ Pushed Thanks Steven >> 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
From 4fe9e7e26ca34dd5eb4bc433f1a1e09075c821fa Mon Sep 17 00:00:00 2001 From: Adrian Guzowski <adrian-007@o2.pl> Date: Mon, 17 Dec 2018 23:14:53 +0100 Subject: [PATCH] Fix usage of temp_file flag in hls_flags option. This is a regression introduced by 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6. It appears that regression was introduced in 4.1, 4.0.x does not share this behaviour. Temp files were not created for MPEG-TS segments options - HLS_TEMP_FILE flag was never set on AVFormatContext, it is however set on HLSContext object. In order to fix this issue, proper flags field must be checked. In addition, renaming code was messed up and apparently was working only for MP4 files. --- libavformat/hlsenc.c | 60 ++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index bdd2a113bd..eec41a0d97 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -1357,8 +1357,9 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs) int ret = 0; char temp_filename[1024]; int64_t sequence = FFMAX(hls->start_sequence, vs->sequence - vs->nb_entries); - const char *proto = avio_find_protocol_name(s->url); - int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE); + const char *proto = avio_find_protocol_name(vs->m3u8_name); + int is_file_proto = proto && !strcmp(proto, "file"); + int use_temp_file = is_file_proto && ((hls->flags & HLS_TEMP_FILE) || !(hls->pl_type == PLAYLIST_TYPE_VOD)); static unsigned warned_non_file; char *key_uri = NULL; char *iv_string = NULL; @@ -1381,7 +1382,7 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs) hls->version = 7; } - if (!use_temp_file && !warned_non_file++) + if (!is_file_proto && (hls->flags & HLS_TEMP_FILE) && !warned_non_file++) av_log(s, AV_LOG_ERROR, "Cannot use rename on non file protocol, this may lead to races and temporary partial files\n"); set_http_options(s, &options, hls); @@ -1477,8 +1478,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs) AVFormatContext *oc = vs->avf; AVFormatContext *vtt_oc = vs->vtt_avf; AVDictionary *options = NULL; - const char *proto = avio_find_protocol_name(s->url); - int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE); + const char *proto = NULL; + int use_temp_file = 0; char *filename, iv_string[KEYSIZE*2 + 1]; int err = 0; @@ -1574,6 +1575,9 @@ static int hls_start(AVFormatContext *s, VariantStream *vs) set_http_options(s, &options, c); + proto = avio_find_protocol_name(oc->url); + use_temp_file = proto && !strcmp(proto, "file") && (c->flags & HLS_TEMP_FILE); + if (use_temp_file) { char *new_name = av_asprintf("%s.tmp", oc->url); if (!new_name) @@ -2142,8 +2146,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt) int ret = 0, can_split = 1, i, j; int stream_index = 0; int range_length = 0; - const char *proto = avio_find_protocol_name(s->url); - int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE); + const char *proto = NULL; + int use_temp_file = 0; uint8_t *buffer = NULL; VariantStream *vs = NULL; AVDictionary *options = NULL; @@ -2254,19 +2258,22 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt) } } + if (oc->url[0]) { + proto = avio_find_protocol_name(oc->url); + use_temp_file = proto && !strcmp(proto, "file") && (hls->flags & HLS_TEMP_FILE); + } + // look to rename the asset name - if (use_temp_file && oc->url[0]) { + if (use_temp_file) { if (!(hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size <= 0)) - if ((vs->avf->oformat->priv_class && vs->avf->priv_data) && hls->segment_type != SEGMENT_TYPE_FMP4) { + if ((vs->avf->oformat->priv_class && vs->avf->priv_data) && hls->segment_type != SEGMENT_TYPE_FMP4) av_opt_set(vs->avf->priv_data, "mpegts_flags", "resend_headers", 0); - } } if (hls->segment_type == SEGMENT_TYPE_FMP4) { if (hls->flags & HLS_SINGLE_FILE) { ret = flush_dynbuf(vs, &range_length); if (ret < 0) { - av_free(old_filename); return ret; } vs->size = range_length; @@ -2284,20 +2291,13 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt) return ret; } ff_format_io_close(s, &vs->out); - - // rename that segment from .tmp to the real one - if (use_temp_file && oc->url[0]) { - hls_rename_temp_file(s, oc); - av_free(old_filename); - old_filename = av_strdup(vs->avf->url); - - if (!old_filename) { - return AVERROR(ENOMEM); - } - } } } + if (use_temp_file && !(hls->flags & HLS_SINGLE_FILE)) { + hls_rename_temp_file(s, oc); + } + old_filename = av_strdup(vs->avf->url); if (!old_filename) { return AVERROR(ENOMEM); @@ -2366,8 +2366,8 @@ static int hls_write_trailer(struct AVFormatContext *s) AVFormatContext *oc = NULL; AVFormatContext *vtt_oc = NULL; char *old_filename = NULL; - const char *proto = avio_find_protocol_name(s->url); - int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE); + const char *proto = NULL; + int use_temp_file = 0; int i; int ret = 0; VariantStream *vs = NULL; @@ -2378,6 +2378,7 @@ static int hls_write_trailer(struct AVFormatContext *s) oc = vs->avf; vtt_oc = vs->vtt_avf; old_filename = av_strdup(vs->avf->url); + use_temp_file = 0; if (!old_filename) { return AVERROR(ENOMEM); @@ -2421,15 +2422,20 @@ static int hls_write_trailer(struct AVFormatContext *s) failed: av_write_trailer(oc); + + if (oc->url[0]) { + proto = avio_find_protocol_name(oc->url); + use_temp_file = proto && !strcmp(proto, "file") && (hls->flags & HLS_TEMP_FILE); + } + if (oc->pb) { if (hls->segment_type != SEGMENT_TYPE_FMP4) { vs->size = avio_tell(vs->avf->pb) - vs->start_pos; - } - if (hls->segment_type != SEGMENT_TYPE_FMP4) ff_format_io_close(s, &oc->pb); + } // rename that segment from .tmp to the real one - if (use_temp_file && oc->url[0] && !(hls->flags & HLS_SINGLE_FILE)) { + if (use_temp_file && !(hls->flags & HLS_SINGLE_FILE)) { hls_rename_temp_file(s, oc); av_free(old_filename); old_filename = av_strdup(vs->avf->url); -- 2.19.2