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

[Peter Cordes]

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.

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.