[FFmpeg-devel] Patch review: av_dict: add support for empty values

[Michael Niedermayer]

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>
> 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.

applied this and 3 other simple patches
thanks

[...]

-- 
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
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150304/7805e02d/attachment.asc>