[FFmpeg-devel] [PATCH] mpeg4video: Add support for MPEG-4 Simple Studio Profile.

Michael Niedermayer michael at niedermayer.cc
Sat Jan 27 23:14:41 EET 2018


On Sat, Jan 27, 2018 at 02:24:29AM +0000, Kieran Kunhya wrote:
> On Sat, Jan 27, 2018 at 12:33 AM, Michael Niedermayer <
> michael at niedermayer.cc> wrote:
> 
> > On Fri, Jan 26, 2018 at 08:04:39PM +0000, Kieran Kunhya wrote:
> > > On Mon, Jan 22, 2018 at 2:07 AM, Michael Niedermayer
> > <michael at niedermayer.cc
> > > > wrote:
> > >
> > > > On Sun, Jan 21, 2018 at 12:37:21PM +0000, Kieran Kunhya wrote:
> > > > > On Mon, Jan 1, 2018 at 7:01 PM, Michael Niedermayer
> > > > <michael at niedermayer.cc>
> > > > > wrote:
> > > > >
> > > > > > Hi
> > > > >
> > > > >
> > > >
> > > > > Patch updated.
> > > > > Some of the review comments I decided not to implement in order to
> > keep
> > > > > closer to the spec.
> > > >
> > > > honestly, this reasoning makes no sense to me.
> > > > that spec does IIRC not even fully describe some parts in DPCM
> > > >
> > > > also all existing mpeg&h26x code we have was designed for speed
> > > > basically ignoring if its close to some spec implementation style or
> > not
> > > >
> > > > I would prefer we try to make our code as good as we can and not try to
> > > > be similar to some specification or reference implementation.
> > > > except for maybe some niche codecs where performance totally doesnt
> > matter
> > > >
> > >
> > > It does describe DPCM, just in an odd way.
> >
> > The specification i read did not specify DPCM in a way one could implement
> > aka the description was not complete.
> >
> > and it also was not understandable to you as you asked on IRC about it
> > ages ago
> >
> > Jän 22 03:12:54 <kierank>       michaelni: don't know if I am missing
> > something here but does it say anywhere what the length of dpcm_residual is?
> > Jän 22 03:41:31 <kierank>       I'm guessing it should read 8-12?
> > Jän 22 03:41:41 <kierank>       in 14496-2
> > ...
> > Dez 23 01:55:30 <kierank>       atomnuker: any idea how to calculate the
> > length of dpcm_residual
> > Dez 23 01:57:19 <kierank>       it just says "dpcm_residual: This is an
> > unsigned integer that indicates the value of a DPCM residual"
> > Dez 23 01:58:08 <nevcairiel>    shouldnt the definiton of "uimsbf" tell
> > you how to parse those values
> > Dez 23 01:58:25 <kierank>       sure, but doesn't explain the 4-12
> > Dez 23 01:58:44 <nevcairiel>    how did you figure out the length of say
> > block_mean. its 8-12 uimsbf
> > Dez 23 01:59:10 <kierank>       block mean is the average of the block and
> > the supported bitdepths are 8-12
> > Dez 23 02:01:31 <nevcairiel>    so basically the format syntax is just
> > bonkers
> > Dez 23 02:10:12 <kierank>       there must be something I'm missing
> >
> > If the spec makes sense to you now, please explain because it made no sense
> > to me last i looked.
> > I genuinely do not understand what it meant
> 
> 
> It is possible to implement, I will send a patch when I am done. Please do
> not quote things from 2017, it is now 2018.

can you explain how you figured out how it works?
Iam really genuinely interrested in understanding how what is written in
the spec explains how to do it, if that is what the implementation is based
on.


> 
> 
> > > I am going to implement your ricing changes but for the record your
> > passive
> > > aggressiveness to many developers is awful.
> >
> > i appologize if something i said sounded passive aggerssive, it was not
> > intended
> > to.
> >
> >
> > > Please learn to compromise.
> >
> > you refer to myself pushing towards a optimial implementation and not the
> > spec?
> > as in " I would prefer we try to make our code as good as we can and not
> > try to
> > be similar to some specification or reference implementation."
> > ?
> >
> > Dont you agree that this is the better choice for teh codebase even though
> > yes
> > it is certainly more work ?
> >
> 
> Changing two lines of code to be less readable and "faster" makes such
> little difference 

> but instead you bully contributors on a daily basis
> backed up by your nasty lieutenants.

That sounds like a insult and slander.

and what or who should lieutenants be ?!
this makes no sense at all


> When will you realise your inability to compromise is a major reason why
> this project is dysfunctional and viewed as a joke in the rest of the OSS
> world.

The problem with this claim is that i give in to almost every request people
have made, so this is quite far out of whack with reality.

But more so iam not the project nor its leader. 

Also about this specific case in this thread. If you prefer to keep some code
closer to the spec. I really have no big issue with that we can go that direction
if you prefer.
But really this is very odd, we have always cleaned up code and simplified it
similarly every other project i can think of does as well.

why all of a sudden does this change seems to matter so much ?

For reference the 2 differences i found are:

         if (ctx->shape != BIN_ONLY_SHAPE)
             s->qscale = get_qscale(s);
 
-        if (show_bits1(gb)) {
-            skip_bits1(gb);   /* slice_extension_flag */
+        if (get_bits1(gb)) {  /* slice_extension_flag */
             skip_bits1(gb);   /* intra_slice */
             skip_bits1(gb);   /* slice_VOP_id_enable */
             skip_bits(gb, 6); /* slice_VOP_id */
-            while (show_bits1(gb)) {
-                skip_bits1(gb);   /* extra_bit_slice */
-                skip_bits(gb, 8); /* slice_VOP_id */
-            }
+            while (get_bits1(gb)) /* extra_bit_slice */
+                skip_bits(gb, 8); /* extra_information_slice */
         }
-        skip_bits1(gb); /* extra_bit_slice */
 
         reset_studio_dc_predictors(s);
     }
@@ -1837,7 +1836,7 @@ static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n
         quant_matrix = s->intra_matrix;
     }
     else {
-        cc = ((n - 4) % 2) + 1; /* Table 7-30 */
+        cc = (n & 1) + 1;
         if (ctx->rgb)
             dct_dc_size = get_vlc2(&s->gb, ctx->studio_luma_dc.table, STUDIO_INTRA_BITS, 2);
         else


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

There will always be a question for which you do not know the correct answer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180127/cc7d128f/attachment.sig>


More information about the ffmpeg-devel mailing list