headers
The Wanderer | 1 Jul 2008 01:14
Picon

Re: [PATCH] Document opt.h:av_find_opt()

Stefano Sabatini wrote:

> Hi all,
> continues the opt.h documentation saga.
> 
> I took account in this patch to all the previous remarks done by
> Diego, Michael, The Wanderer as much as my mind made it possible ;-).

Not bad, but I see a couple of possible improvements. (This is almost on 
the level of nitpicking, but I points 'em out as I sees 'em...)

> + * Looks for an option in \p obj. Looks only for the options which
> + * have the flags set as specified in \p mask and \p flags (that is,
> + * for which is: opt->flags & mask == flags).

I would probably say something more like "for which it is the case that"
(or, less precisely but more simply, just "for which") and drop the
colon.

> + *  <at> param[in] obj a pointer to an #AVClass struct or to an #AVClass
> + * context struct
> + *  <at> param[in] name the name of the option to look for
> + *  <at> param[in[ unit the unit of the option to look for or any if NULL
> + *  <at> return a pointer to the option found or NULL if no option
> + * has been found

I would add a comma before the "or" on both of these last two.

Other than that, looks fairly good to me.
(Continue reading)

Permalink | Reply | 11 comments |
headers
Stefano Sabatini | 1 Jul 2008 01:32
Picon
Favicon

Re: [PATCH] Document opt.h:av_find_opt()

On date Monday 2008-06-30 19:14:24 -0400, The Wanderer encoded:
> Stefano Sabatini wrote:
> 
> > Hi all,
> > continues the opt.h documentation saga.
> > 
> > I took account in this patch to all the previous remarks done by
> > Diego, Michael, The Wanderer as much as my mind made it possible ;-).
> 
> Not bad, but I see a couple of possible improvements. (This is almost on 
> the level of nitpicking, but I points 'em out as I sees 'em...)
> 
> > + * Looks for an option in \p obj. Looks only for the options which
> > + * have the flags set as specified in \p mask and \p flags (that is,
> > + * for which is: opt->flags & mask == flags).
> 
> I would probably say something more like "for which it is the case that"
> (or, less precisely but more simply, just "for which") and drop the
> colon.
> 
> > + *  <at> param[in] obj a pointer to an #AVClass struct or to an #AVClass
> > + * context struct
> > + *  <at> param[in] name the name of the option to look for
> > + *  <at> param[in[ unit the unit of the option to look for or any if NULL
> > + *  <at> return a pointer to the option found or NULL if no option
> > + * has been found
> 
> I would add a comma before the "or" on both of these last two.

Mmh... this still continues to seem strange to me, nonetheless I
(Continue reading)

Permalink | Reply | 10 comments |
headers
Michael Niedermayer | 1 Jul 2008 02:39
Picon
Picon

Re: [PATCH] MVI demuxer / Motion Pixels decoder

On Sun, Jun 29, 2008 at 11:23:10PM +0200, Gregory Montoir wrote:
>
> Attached patch adds basic support for motion pixels .mvi files. I wanted to 
> be able to transcode my old files ; so here are the changes, if there's 
> interest.
>
> I also uploaded a sample (intro.mvi) to /incoming.
>
> Regards,
> Gregory

[...]
> +typedef struct MotionPixelsContext {
> +    AVCodecContext *avctx;
> +    AVFrame frame;

> +    DeltaAcc *quant_table;

as that table is constant it should be static const and not duplicated for
each instance

[...]
> +static void mp_read_changes_map1(MotionPixelsContext *mp, GetBitContext *gb, int count, int bits_len)
> +{
> +    int offset, w, h;
> +
> +    while (count--) {
> +        offset = get_bits_long(gb, mp->offset_bits_len);
> +        w      = get_bits(gb, bits_len) + 1;
> +        h      = get_bits(gb, bits_len) + 1;
(Continue reading)

Permalink | Reply | 14 comments |
headers
Ramiro Polla | 1 Jul 2008 02:55
Picon
Favicon

Re: [PATCH] MLP/TrueHD Decoder - 2nd try

Hello Michael,

Michael Niedermayer wrote:
> On Sun, Jun 29, 2008 at 12:41:17PM +0100, Ramiro Polla wrote:
>> Hello,
>>
> [...]
>> mlpdec.c is Ian's MLP/TrueHD decoder with a few modifications as can be 
>> seen in the SoC tree.
>>
> 
>> Valgrind doesn't complain about invalid writes on a bunch of trashed files 
>> and some manually edited files that pass the checks as well.
> 
> I assume that you disabled all checksum checks for these tests?

Oops. A bunch of more tests run with checksums disabled. Still no 
problem seen.

> [...]
>> static const char* sample_message =
>>     "Please file a bug report following the instructions at "
>>     "http://ffmpeg.mplayerhq.hu/bugreports.html and include "
>>     "a sample of this file.";
> 
> Dont we already have something like that somewhere? Or was that another
> unfinished patch, either way above can be shared wth other codecs ...

IIRC it's in Robert's AAC patch. I haven't checked the archives to make 
sure though. Anyways I think it's not in SVN yet.
(Continue reading)

Permalink | Reply | 25 comments |
headers
Michael Niedermayer | 1 Jul 2008 02:55
Picon
Picon

Re: [PATCH] Document opt.h:av_find_opt()

On Tue, Jul 01, 2008 at 01:32:10AM +0200, Stefano Sabatini wrote:
> On date Monday 2008-06-30 19:14:24 -0400, The Wanderer encoded:
> > Stefano Sabatini wrote:
> > 
> > > Hi all,
> > > continues the opt.h documentation saga.
> > > 
> > > I took account in this patch to all the previous remarks done by
> > > Diego, Michael, The Wanderer as much as my mind made it possible ;-).
> > 
> > Not bad, but I see a couple of possible improvements. (This is almost on 
> > the level of nitpicking, but I points 'em out as I sees 'em...)
> > 
> > > + * Looks for an option in \p obj. Looks only for the options which
> > > + * have the flags set as specified in \p mask and \p flags (that is,
> > > + * for which is: opt->flags & mask == flags).
> > 
> > I would probably say something more like "for which it is the case that"
> > (or, less precisely but more simply, just "for which") and drop the
> > colon.
> > 
> > > + *  <at> param[in] obj a pointer to an #AVClass struct or to an #AVClass
> > > + * context struct
> > > + *  <at> param[in] name the name of the option to look for
> > > + *  <at> param[in[ unit the unit of the option to look for or any if NULL
> > > + *  <at> return a pointer to the option found or NULL if no option
> > > + * has been found
> > 
> > I would add a comma before the "or" on both of these last two.
>
(Continue reading)

Permalink | Reply | 9 comments |
headers
Ramiro Polla | 1 Jul 2008 03:09
Picon
Favicon

Re: [PATCH] MLP/TrueHD Decoder - 2nd try

Michael Niedermayer wrote:
> On Sun, Jun 29, 2008 at 12:41:17PM +0100, Ramiro Polla wrote:
>> Hello,
>>
>> 01_mlp_parser_gb.diff makes a function in mlp_parser receive a 
>> GetBitContext instead of uint8_t buffers. This helps out in the decoder to 
>> simplify the code.
> 
> ok, after fixing issue below

[...]

>> -int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, const uint8_t *buf,
>> -                           unsigned int buf_size)
>> +int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, GetBitContext *gb)
>>  {
>> -    GetBitContext gb;
>>      int ratebits;
>>      uint16_t checksum;
>>  
>> -    if (buf_size < 28) {
>> +    assert(get_bits_count(gb) == 0);
>> +
>> +    if (gb->size_in_bits < 28 << 3) {
>>          av_log(log, AV_LOG_ERROR, "Packet too short, unable to read major sync\n");
>>          return -1;
>>      }
>>  
>> -    checksum = mlp_checksum16(buf, 26);
>> -    if (checksum != AV_RL16(buf+26)) {
(Continue reading)

Permalink | Reply | 2 comments |
headers
Michael Niedermayer | 1 Jul 2008 03:15
Picon
Picon

Re: [PATCH] MLP/TrueHD Decoder - 2nd try

On Tue, Jul 01, 2008 at 02:09:10AM +0100, Ramiro Polla wrote:
> Michael Niedermayer wrote:
>> On Sun, Jun 29, 2008 at 12:41:17PM +0100, Ramiro Polla wrote:
>>> Hello,
>>>
>>> 01_mlp_parser_gb.diff makes a function in mlp_parser receive a 
>>> GetBitContext instead of uint8_t buffers. This helps out in the decoder 
>>> to simplify the code.
>> ok, after fixing issue below
>
> [...]
>
>>> -int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, const uint8_t 
>>> *buf,
>>> -                           unsigned int buf_size)
>>> +int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, GetBitContext 
>>> *gb)
>>>  {
>>> -    GetBitContext gb;
>>>      int ratebits;
>>>      uint16_t checksum;
>>>  -    if (buf_size < 28) {
>>> +    assert(get_bits_count(gb) == 0);
>>> +
>>> +    if (gb->size_in_bits < 28 << 3) {
>>>          av_log(log, AV_LOG_ERROR, "Packet too short, unable to read 
>>> major sync\n");
>>>          return -1;
>>>      }
>>>  -    checksum = mlp_checksum16(buf, 26);
(Continue reading)

Permalink | Reply | 1 comment |
headers
Ramiro Polla | 1 Jul 2008 03:36
Picon
Favicon

Re: [PATCH] MLP/TrueHD Decoder - 2nd try

Michael Niedermayer wrote:
> On Tue, Jul 01, 2008 at 02:09:10AM +0100, Ramiro Polla wrote:
>> Michael Niedermayer wrote:
>>> On Sun, Jun 29, 2008 at 12:41:17PM +0100, Ramiro Polla wrote:
>>>> Hello,
>>>>
>>>> 01_mlp_parser_gb.diff makes a function in mlp_parser receive a 
>>>> GetBitContext instead of uint8_t buffers. This helps out in the decoder 
>>>> to simplify the code.
>>> ok, after fixing issue below
>> [...]
>>
>>>> -int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, const uint8_t 
>>>> *buf,
>>>> -                           unsigned int buf_size)
>>>> +int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, GetBitContext 
>>>> *gb)
>>>>  {
>>>> -    GetBitContext gb;
>>>>      int ratebits;
>>>>      uint16_t checksum;
>>>>  -    if (buf_size < 28) {
>>>> +    assert(get_bits_count(gb) == 0);
>>>> +
>>>> +    if (gb->size_in_bits < 28 << 3) {
>>>>          av_log(log, AV_LOG_ERROR, "Packet too short, unable to read 
>>>> major sync\n");
>>>>          return -1;
>>>>      }
>>>>  -    checksum = mlp_checksum16(buf, 26);
(Continue reading)

Permalink | Reply | 0 comments |
headers
Michael Niedermayer | 1 Jul 2008 03:38
Picon
Picon

Re: [PATCH] MLP/TrueHD Decoder - 2nd try

On Tue, Jul 01, 2008 at 01:55:39AM +0100, Ramiro Polla wrote:
> Hello Michael,
[...]
>>>     // <at> {
>>>     /** Restart header data */
>>>     //! The sync word used at the start of the last restart header
>>>     uint16_t    restart_sync_word[MAX_SUBSTREAMS];
>>>
>>>     //! The index of the first channel coded in this substream
>>>     uint8_t     min_channel[MAX_SUBSTREAMS];
>>>     //! The index of the last channel coded in this substream
>>>     uint8_t     max_channel[MAX_SUBSTREAMS];
>>>     //! The number of channels input into the rematrix stage
>>>     uint8_t     max_matrix_channel[MAX_SUBSTREAMS];
>>>
>>>     //! The left shift applied to random noise in 0x31ea substreams
>>>     uint8_t     noise_shift[MAX_SUBSTREAMS];
>>>     //! The current seed value for the pseudorandom noise generator(s)
>>>     uint32_t    noisegen_seed[MAX_SUBSTREAMS];
>>>
>>>     //! Does this substream contain extra info to check the size of VLC 
>>> blocks?
>>>     uint8_t     data_check_present[MAX_SUBSTREAMS];
>>>
>>>     //! Bitmask of which parameter sets are conveyed in a decoding 
>>> parameter block
>>>     uint8_t     param_presence_flags[MAX_SUBSTREAMS];
>>> #define PARAM_BLOCKSIZE     (1 << 7)
>>> #define PARAM_MATRIX        (1 << 6)
>>> #define PARAM_OUTSHIFT      (1 << 5)
(Continue reading)

Permalink | Reply | 24 comments |
headers
Siarhei Siamashka | 1 Jul 2008 09:30
Picon

Re: [PATCH] Fix mm_flags, mm_support for ARM

On Monday 30 June 2008, matthieu castet wrote:
> > Could you or anybody else having compatible ARM device just do some
> > benchmarking to confirm my results (I posted benchmarks here multiple
> > times already). It would be a really good help. Because I feel that
> > some people here still doubt that it provides a major performance
> > improvement.
>
> For dct-test (yes I know it is not a benchmark) on a arm926ejs svn
> implementation got 126.7 kdct/s, your 154.6 kdct/s.

In addition, current SVN implementation does not skip over empty coefficients
on columns processing, this gives it a slight advantage in dct-test. Special
handling of sparse data slows down the worst case a bit (such as dct-test) for
the new proposed ARMv5TE IDCT, but improves performance on typical data when
decoding video.

It is better to benchmark overall performance improvement on decoding some
video files.

Also it is not a problem with current SVN implementation, but it was optimized
for ARM9E only. And I made sure that updated IDCT is also fast on cores with
longer pipeline (and higher latencies) such as ARM11 and XScale. From the
practical point of view, there exist older XScale cores without iWMMXt support
(PXA255). And newer XScale cores (PXA27x or better) do not have iWMMXt
optimized IDCT in FFmpeg yet. It is possible to use IPP for getting iWMMXt
optimizations, but IPP has non-GPL compatible license and some distributions
prefer to avoid it.

> > Once/if the performance improvement is confirmed, a help with integration
> > would be really needed. That's not a joke, I really fail to see any
(Continue reading)

Permalink | Reply | 2 comments |

Gmane