2017-11-14 23:48 GMT+01:00 Carl Eugen Hoyos <ceffm...@gmail.com>: > 2017-11-13 21:07 GMT+01:00 Thilo Borgmann <thilo.borgm...@mail.de>: >> Am 13.11.17 um 21:06 schrieb Thilo Borgmann: >>> Am 13.11.17 um 20:02 schrieb Umair Khan: >>>> Hi, >>>> >>>> On Mon, Nov 13, 2017 at 11:06 PM, Thilo Borgmann <thilo.borgm...@mail.de> >>>> wrote: >>>>> Hi, >>>>> >>>>>> On Mon, Nov 13, 2017 at 1:09 AM, Carl Eugen Hoyos <ceffm...@gmail.com> >>>>>> wrote: >>>>>>> 2017-11-12 20:30 GMT+01:00 Umair Khan <omerj...@gmail.com>: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Mon, Nov 13, 2017 at 12:45 AM, Carl Eugen Hoyos >>>>>>>> <ceffm...@gmail.com> wrote: >>>>>>>>> 2017-11-12 20:05 GMT+01:00 Umair Khan <omerj...@gmail.com>: >>>>>>>>> >>>>>>>>>> The attached patch fixes the address sanitizer issue. >>>>>>>>> >>>>>>>>> Breaks compilation here, how did you test? >>>>>>>>> >>>>>>>>> libavcodec/alsdec.c: In function ‘decode_var_block_data’: >>>>>>>>> libavcodec/alsdec.c:938:7: error: expected ‘}’ before ‘else’ >>>>>>>> >>>>>>>> Sorry for the faulty patch. Here is the fixed one. >>>>>>> >>>>>>> The commit message of your patch is: >>>>>>> libavcodec/als: fix address sanitization error in decoder >>>>>>> >>>>>>> Is there an error in current FFmpeg git head that asan >>>>>>> shows? If not, the commit message makes no sense. >>>>>>> >>>>>>> I believe you should send two patches that are meant >>>>>>> to be committed together, one of them fixing ticket #6297. >>>>>> >>>>>> This is the complete patchset. >>>>> >>>>> I need some days to find time to test this, earliest during the weekend I >>>>> fear... >>>>> >>>>> What happens for >>>>> block_length < residual_index < opt_order? >>>> >>>> I didn't really understand this case. What's residual_index? Can you >>>> point to the source exactly? >>>> >>>> As far as the case where opt_order is more than block_length, my >>>> second patch handles that case only. The file which Michael sent was >>>> having asan issues because of the case when block_length < opt_order. >>>> >>>>> Another way of asking would be, where is the second loop from specs page >>>>> 30 for that case? >>>>> (ISO/IEC 14496) >>>> >>>> The second loop is just converting parcor to lpc coefficients which is >>>> done here - >>>> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/alsdec.c#L935 >>>> >>>>> I think what puzzles CE is, that the problematic if() from the other >>>>> patches is still untouched by your patch. So how could this be a valid >>>>> solution even if your patch would actually improve the prediction part... >>>>> And I wonder the same ;) >>>> >>>> As said, it is valid to have opt_order greater than block_length. >>>> However, the decoder loop needs to be checked because we won't predict >>>> values more than the length of the block i.e., block_length. We use >>>> last K (prediction order, opt_order) values to predict the original K >>>> values of the current block. >>>> >>>>> Did you run FATE with your patch applied? I assume a big difference in >>>>> output at the first glance (means FATE aks the conformance files should >>>>> fail...) >>>> >>>> Yes. I did run FATE. It passes perfectly. >>>> >>>>> Thanks for driving this forward anyway :) >>>> >>>> I think the two patches fix the issues completely. I don't see any >>>> harm in applying this patchset. :) >>> >>> Which second patch exactly do you want to be applied along with this one? >> >> For convenience just send a patch that does both the changes you want to be >> done...
Sorry: Why do you want to merge two patches that are not necessarily related: Isn't one the revert that fixes a regression and one the new fix for the overread. Carl Eugen _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel