On Wed, Mar 4, 2015 at 9:42 AM, Michael Niedermayer <michae...@gmx.at> wrote:
> On Wed, Mar 04, 2015 at 02:24:58PM +0100, Michael Niedermayer wrote: > > On Wed, Mar 04, 2015 at 02:11:42PM +0100, Michael Niedermayer wrote: > > > On Tue, Mar 03, 2015 at 10:51:01PM -0400, Peter Cordes wrote: > > > [...] > > > > Anyway, the av_dict change is the one that requires the most review, > so > > > > I'll make this email focus on that set of changes. > > > > https://github.com/FFmpeg/FFmpeg/pull/118 > > > > > > pull req #3, patch #1 review > > > > > > > - char *ret = out, *end = out; > > > > + char *ret = out, *end_quote = out; > > > > > > why ? > Because it took me a couple minutes to see that was all "end" was doing. I wasn't sure what it was the end of. (At first, I didn't even realize that this function was handling quoting as well as tokenizing.) > > > + ret = av_realloc(ret, out - ret + 2); > > > > + // if realloc fails to shrink, we're hosed anyway; just leak > the old buffer > > > > return ret; > Yeah, I should have collapsed that bad idea / fix before pushing. I wanted to leave the code shorter, but then I realized I was starting to write a whole paragraph in a commit message to justify a one-line shortcut, so it was probably a bad idea. :P > > if realloc fails to shrink, the original unshrunk array should be > > > returned to avoid the leak and failure > > > > ahh, i see you fix this in a later commit, this should be stashed > > with the patch that would add the bug if it was pushed > > and now i see you mentioned that in your mail, i guess replying to > the first part and then reviewing some of the code before reading > the rest of the mail was not really a great idea I have ADHD, I have a hard time keeping my emails under 10 paragraphs of different ideas. :P My main desktop just developed some sort of instability, maybe hardware. It might be a few days before I update these patches, if I don't get to it on my laptop. I guess ffmpeg prefers splitting changes into MANY separate commits for more accurate bisection. I knew that, but didn't realize what level of splitting up was aimed for. Will do for all my other patches, now that I have a better idea of what might actually get accepted. (esp. the libx264.c commit has at least 3 commits worth of changes.) Several of the patches on my other branches are already single-item changes that are ready to go in, though. (Other than the mpdecimate one where I thought there was a bug passing passing an invalid pointer as a log context. I'm still learning my way around ffmpeg. Thanks for the patch review on that.) Especially the vf_showinfo change is simple and useful. I also have a patch I didn't put on github; a non-working attempt to enable 10bit libx264rgb (it crashes). If anyone's already tried high-depth libx264rgb, I'm not finding it easily with google. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel