[FFmpeg-devel] [PATCH] extract bit rate calculation into separate function

[Michael Niedermayer]

On Fri, Nov 13, 2009 at 11:42:30PM +0100, Robert Kr?ger wrote:
>
> On 13.11.2009, at 19:02, Michael Niedermayer wrote:
>
>> On Fri, Nov 13, 2009 at 08:59:16AM +0100, Robert Kr?ger wrote:
>>>
>>> On 12.11.2009, at 19:29, Michael Niedermayer wrote:
>>>
>>>> On Thu, Nov 12, 2009 at 09:13:58AM +0100, Robert Kr?ger wrote:
>>>>> Hi,
>>>>>
>>>>> On 12.11.2009, at 01:42, Stefano Sabatini wrote:
>>>>>
>>>>>> On date Wednesday 2009-11-11 12:41:33 +0100, Robert Kr?ger encoded:
>>>> [...]
>>>>>>> +
>>>>> hmm, I tried not to change the behaviour of the code I extracted as I
>>>>> only
>>>>> wanted to refactor and what you suggest would change the behaviour for
>>>>> CODEC_TYPE_UNKNOWN ,  CODEC_TYPE_ATTACHMENT and CODEC_TYPE_NB as it 
>>>>> would
>>>>> no longer return 0 but return ctx->bit_rate. If that makes sense, I
>>>>> cannot
>>>>> judge. I'll change it accordingly, if you say it does.
>>>>
>>>> you could send 2 pathes
>>>> first to move the code, second to simplify it
>>>>
>>> OK, then consider the first one submitted with my most recent email.
>>
>> your most recent mail adds code duplication, the previous patches are
> Hmmm, the code duplication must refer to the fact that my most recent patch 
> only adds the new function without removing the code from the function 
> where I extracted it. That's how I understood Stefano I should do it, i.e. 
> send the one patch with the new function, have it accepted, then send the 
> one eliminating the duplicate code, which I thought I just did. Please tell 
> me what I should do to do it right so that someone who can make that call 
> will accept it.

patch 1:
Move the existing code in a new function (this IIRC is pretty much your first
patch with minor changes like tabs/line breaks so it should be littel work)
(This should not change behaviour, any change of behaviour would be a bug)

patch 2:
simplify our quite crappy code (you also have done that already pretty much
in your latest patch)
(In this patch we can see exactly what code is changed and the reviewer (me)
can easily see if the new code does what the old did in all relevant cases
and if more things can be improved ...)



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091114/1e1256ff/attachment.pgp>