[FFmpeg-devel] Patch review: av_dict: add support for empty values
michaelni at gmx.at
Wed Mar 4 22:16:01 CET 2015
On Wed, Mar 04, 2015 at 02:19:36PM -0400, Peter Cordes wrote:
> On Wed, Mar 4, 2015 at 9:42 AM, Michael Niedermayer <michaelni at gmx.at>
> > 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.
applied this and 3 other simple patches
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 1
"Used only once" - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 181 bytes
Desc: Digital signature
More information about the ffmpeg-devel