headers
Michael Niedermayer | 1 Aug 2009 01:11
Picon
Picon

Re: [PATCH] Set correct frame_size for Speex decoding

On Fri, Jul 31, 2009 at 06:48:59PM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Thu, Jul 30, 2009 at 07:25:17PM -0400, Justin Ruggles wrote:
> >> Justin Ruggles wrote:
> >>> Hi,
> >>>
> >>> Currently AVCodecContext.frame_size is not set correctly for Speex.
> >>> Since the Ogg and FLV demuxers and the libspeex decoder handle a full
> >>> packet as a single frame, frame_size should be set to the Speex
> >>> frame_size * frames_per_packet.
> >>>
> >>> If frames_per_packet is not specified in the Speex header, or if there
> >>> is no header, it can be determined after decoding the first packet.
> >>>
> >>> Stream copy is not implemented yet for Speex, but once it is, a parser
> >>> will be able to set all the stream parameters instead of the decoder
> >>> when the header is missing or incomplete.
> >> ping.
> > 
> > it might be helpfull if you say who you expect to review this
> 
> In general, I'm looking for an ok on having lavf and lavc treat a whole
> Speex packet as a single frame.  After considering and trying to code
> the split-then-join idea it did not seem like a very clean solution, and
> it is not really necessary.  This is my general plan:

which values of frames_per_packet does each container allow?
that is each container that supports speex

[...]
(Continue reading)

Permalink | Reply | 24 comments |
headers
Justin Ruggles | 1 Aug 2009 01:34
Picon
Gravatar

Re: [PATCH] Set correct frame_size for Speex decoding

Michael Niedermayer wrote:
> On Fri, Jul 31, 2009 at 06:48:59PM -0400, Justin Ruggles wrote:
>> Michael Niedermayer wrote:
>>> On Thu, Jul 30, 2009 at 07:25:17PM -0400, Justin Ruggles wrote:
>>>> Justin Ruggles wrote:
>>>>> Hi,
>>>>>
>>>>> Currently AVCodecContext.frame_size is not set correctly for Speex.
>>>>> Since the Ogg and FLV demuxers and the libspeex decoder handle a full
>>>>> packet as a single frame, frame_size should be set to the Speex
>>>>> frame_size * frames_per_packet.
>>>>>
>>>>> If frames_per_packet is not specified in the Speex header, or if there
>>>>> is no header, it can be determined after decoding the first packet.
>>>>>
>>>>> Stream copy is not implemented yet for Speex, but once it is, a parser
>>>>> will be able to set all the stream parameters instead of the decoder
>>>>> when the header is missing or incomplete.
>>>> ping.
>>> it might be helpfull if you say who you expect to review this
>> In general, I'm looking for an ok on having lavf and lavc treat a whole
>> Speex packet as a single frame.  After considering and trying to code
>> the split-then-join idea it did not seem like a very clean solution, and
>> it is not really necessary.  This is my general plan:
> 
> which values of frames_per_packet does each container allow?
> that is each container that supports speex

AFAIK, Ogg is only limited by the Speex header, which supports up to
INT32_MAX. (libspeex reads the 4-byte value in the header as a signed int32)
(Continue reading)

Permalink | Reply | 23 comments |
headers
Michael Niedermayer | 1 Aug 2009 03:50
Picon
Picon

Re: [PATCH][RFC] Indeo3 replacement

On Fri, Jul 31, 2009 at 01:28:21PM +0200, Maxim wrote:
> Michael Niedermayer schrieb:
> > On Mon, Jul 27, 2009 at 05:28:07PM +0200, Maxim wrote:
> >   
> >> Michael Niedermayer schrieb:
> >>     
> >>>>     /* setup output and reference pointers */
> >>>>     dst = &plane->pixels[buf_switch][(cell->ypos << 2) * plane->pitch + (cell->xpos << 2)];
> >>>>     /* reference block = prev_frame(cell_xpos + mv_x, cell_ypos + mv_y) */
> >>>>     mv_y = cell->mv_ptr[0];
> >>>>     mv_x = cell->mv_ptr[1];
> >>>>     offset = ((cell->ypos << 2) + mv_y) * plane->pitch + (cell->xpos << 2) + mv_x;
> >>>>     src = &plane->pixels[buf_switch ^ 1][offset];A typical cell consists of 4x4 blocks in raster order
and is usually
> >>>> 4-6 blocks or 16-24 pixels wide. By INTER pictures it's usually bigger
> >>>>
> >>>>     for (y = cell->height << 2; y > 0; src += plane->pitch, dst += plane->pitch, y--)
> >>>>         memcpy(dst, src, cell->width << 2);
> >>>> }
> >>>>     
> >>>>         
> >>> also, cant the dsputil block copy code be used?
> >>>   
> >>>       
> >> Which function I have to look into? Plz help, I'm not a dsputil's guru...
> >>     
> 
> I'm very sorry but I don't really understand why you ask me to replace
> the code above with block_copy? Is "memcpy" not fast enough? I'm aware
> of the fact that we shouldn't duplicate code but I'm not sure that the
(Continue reading)

Permalink | Reply | 0 comments |
headers
Michael Niedermayer | 1 Aug 2009 04:06
Picon
Picon

Re: [PATCH] Set correct frame_size for Speex decoding

On Fri, Jul 31, 2009 at 07:34:00PM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Fri, Jul 31, 2009 at 06:48:59PM -0400, Justin Ruggles wrote:
> >> Michael Niedermayer wrote:
> >>> On Thu, Jul 30, 2009 at 07:25:17PM -0400, Justin Ruggles wrote:
> >>>> Justin Ruggles wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Currently AVCodecContext.frame_size is not set correctly for Speex.
> >>>>> Since the Ogg and FLV demuxers and the libspeex decoder handle a full
> >>>>> packet as a single frame, frame_size should be set to the Speex
> >>>>> frame_size * frames_per_packet.
> >>>>>
> >>>>> If frames_per_packet is not specified in the Speex header, or if there
> >>>>> is no header, it can be determined after decoding the first packet.
> >>>>>
> >>>>> Stream copy is not implemented yet for Speex, but once it is, a parser
> >>>>> will be able to set all the stream parameters instead of the decoder
> >>>>> when the header is missing or incomplete.
> >>>> ping.
> >>> it might be helpfull if you say who you expect to review this
> >> In general, I'm looking for an ok on having lavf and lavc treat a whole
> >> Speex packet as a single frame.  After considering and trying to code
> >> the split-then-join idea it did not seem like a very clean solution, and
> >> it is not really necessary.  This is my general plan:
> > 
> > which values of frames_per_packet does each container allow?
> > that is each container that supports speex
> 
> AFAIK, Ogg is only limited by the Speex header, which supports up to
(Continue reading)

Permalink | Reply | 22 comments |
headers
Vitor Sessak | 1 Aug 2009 04:33
Picon

Re: [PATCH] TwinVQ decoder

Hi, and thanks for the review

On Fri, Jul 31, 2009 at 11:41 AM, Reimar
Döffinger<Reimar.Doeffinger <at> gmx.de> wrote:
> On Wed, Jul 22, 2009 at 09:16:50AM +0200, Vitor Sessak wrote:
>> New version attached. Changes:
>>
>> - Use DSP functions
>> - Do not alloc big buffers on the stack
>> - Some random optimizations
>> - Diego's cosmetics suggestions
>
> I have not looked at previous reviews...
>
>> /**
>>  * Parameters and tables that are different for each frame type
>>  */
>> typedef struct {
>> } FrameMode;
>
> Used only once, so IMO use only struct FrameMode and don't add a typedef.

I agree.

>> typedef struct {
>>     const FrameMode fmode[3]; ///< frame type-dependant parameters
>
> I think that const makes little sense, for the static tables it
> is const because the overall  struct is const, and like this
> it would make it impossible to initialize fmode.
(Continue reading)

Permalink | Reply | 14 comments |
headers
Geza Kovacs | 1 Aug 2009 06:41
Picon
Favicon

Re: [PATCH] Playlist API


I believe this updated patch, where I'm using a list of AVFormatContext
rather than PlayElem and using a single durations list rather than
per-stream time offsets should address most of issues you pointed out.

>> +            pkt->dts += time_offset;
>> +            if (!ic->streams[pkt->stream_index]->codec->has_b_frames)
>> +                pkt->pts = pkt->dts + 1;
> 
> whatever that is, it is wrong

pkt->dts is offset to ensure that timestamps reflect the actual time the
packet it relative to the concatenated stream, rather than resetting
timestamps to 0 for every new playlist item, so that streams are
appended onto the ends of each other, rather than overlapping (ie if you
don't modify pkt->dts, if you have video 1 with length 5 seconds and
concatenate it with video 2 with length 10 seconds, you only get a video
of length 10 seconds with all 5 seconds of video 1 and the last 5
seconds of video 2).

As for pkt->pts, certain codecs, require that it be greater than the
value of dts, hence it's set to pkt->dts+1, except when using b-frames
in which case it's not modified.

>> +int ff_concatgen_read_seek(AVFormatContext *s,
>> +                           int stream_index,
>> +                           int64_t pts,
>> +                           int flags)
>> +{
>> +    PlaylistContext *ctx;
(Continue reading)

Permalink | Reply | 25 comments |
headers
Justin Ruggles | 1 Aug 2009 12:53
Picon
Gravatar

Re: [PATCH] Set correct frame_size for Speex decoding

Michael Niedermayer wrote:
> On Fri, Jul 31, 2009 at 07:34:00PM -0400, Justin Ruggles wrote:
>> Michael Niedermayer wrote:
>>> On Fri, Jul 31, 2009 at 06:48:59PM -0400, Justin Ruggles wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Thu, Jul 30, 2009 at 07:25:17PM -0400, Justin Ruggles wrote:
>>>>>> Justin Ruggles wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Currently AVCodecContext.frame_size is not set correctly for Speex.
>>>>>>> Since the Ogg and FLV demuxers and the libspeex decoder handle a full
>>>>>>> packet as a single frame, frame_size should be set to the Speex
>>>>>>> frame_size * frames_per_packet.
>>>>>>>
>>>>>>> If frames_per_packet is not specified in the Speex header, or if there
>>>>>>> is no header, it can be determined after decoding the first packet.
>>>>>>>
>>>>>>> Stream copy is not implemented yet for Speex, but once it is, a parser
>>>>>>> will be able to set all the stream parameters instead of the decoder
>>>>>>> when the header is missing or incomplete.
>>>>>> ping.
>>>>> it might be helpfull if you say who you expect to review this
>>>> In general, I'm looking for an ok on having lavf and lavc treat a whole
>>>> Speex packet as a single frame.  After considering and trying to code
>>>> the split-then-join idea it did not seem like a very clean solution, and
>>>> it is not really necessary.  This is my general plan:
>>> which values of frames_per_packet does each container allow?
>>> that is each container that supports speex
>> AFAIK, Ogg is only limited by the Speex header, which supports up to
>> INT32_MAX. (libspeex reads the 4-byte value in the header as a signed int32)
(Continue reading)

Permalink | Reply | 21 comments |
headers
Michael Niedermayer | 1 Aug 2009 13:11
Picon
Picon

Re: [PATCH] Set correct frame_size for Speex decoding

On Sat, Aug 01, 2009 at 06:53:11AM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Fri, Jul 31, 2009 at 07:34:00PM -0400, Justin Ruggles wrote:
> >> Michael Niedermayer wrote:
> >>> On Fri, Jul 31, 2009 at 06:48:59PM -0400, Justin Ruggles wrote:
> >>>> Michael Niedermayer wrote:
> >>>>> On Thu, Jul 30, 2009 at 07:25:17PM -0400, Justin Ruggles wrote:
> >>>>>> Justin Ruggles wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Currently AVCodecContext.frame_size is not set correctly for Speex.
> >>>>>>> Since the Ogg and FLV demuxers and the libspeex decoder handle a full
> >>>>>>> packet as a single frame, frame_size should be set to the Speex
> >>>>>>> frame_size * frames_per_packet.
> >>>>>>>
> >>>>>>> If frames_per_packet is not specified in the Speex header, or if there
> >>>>>>> is no header, it can be determined after decoding the first packet.
> >>>>>>>
> >>>>>>> Stream copy is not implemented yet for Speex, but once it is, a parser
> >>>>>>> will be able to set all the stream parameters instead of the decoder
> >>>>>>> when the header is missing or incomplete.
> >>>>>> ping.
> >>>>> it might be helpfull if you say who you expect to review this
> >>>> In general, I'm looking for an ok on having lavf and lavc treat a whole
> >>>> Speex packet as a single frame.  After considering and trying to code
> >>>> the split-then-join idea it did not seem like a very clean solution, and
> >>>> it is not really necessary.  This is my general plan:
> >>> which values of frames_per_packet does each container allow?
> >>> that is each container that supports speex
> >> AFAIK, Ogg is only limited by the Speex header, which supports up to
(Continue reading)

Permalink | Reply | 20 comments |
headers
Michael Niedermayer | 1 Aug 2009 17:59
Picon
Picon

Re: [PATCH] Expose QCELP's floating-point LSP-to-LPC function

On Fri, Jul 31, 2009 at 09:18:42PM +0100, Colin McQuillan wrote:
> I'd like to use the code in qcelp_lpc.c function for AMR. First, the
> naming should be made consistent.
> 
> rename_qcelp_lsp.patch description:
> 
> Make the LSP naming more consistent
> Use the convention from lsp.c: an LSF is a frequency, an LSP is the
> cosine of an LSF, and LSP functions should have an ff_acelp prefix.
> Use a "d" suffix to specify doubles.

iam not maintainer of qcelp but as iam not sure if reynaldo is busy,
ive not seen a mail from him recently IIRC, and soc time is limited
i approve this one, if reynaldo disagrees it can be reverted

> 
> move_qcelp_lsp.patch description:
> 
> Expose QCELP's floating-point LSP-to-LPC function
> qcelp_lsp exported a single function, ff_acelp_lspd2lpc, which was not
> specific to qcelp. It can be kept with its fixed-point version
> ff_acelp_lsp2lpc in lpc.c.

similarly ok

[...]
--

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
(Continue reading)

Permalink | Reply | 3 comments |
headers
Michael Niedermayer | 1 Aug 2009 18:43
Picon
Picon

Re: [PATCH] Playlist API

On Fri, Jul 31, 2009 at 09:41:54PM -0700, Geza Kovacs wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I believe this updated patch, where I'm using a list of AVFormatContext
> rather than PlayElem and using a single durations list rather than
> per-stream time offsets should address most of issues you pointed out.
> 
> >> +            pkt->dts += time_offset;

> >> +            if (!ic->streams[pkt->stream_index]->codec->has_b_frames)
> >> +                pkt->pts = pkt->dts + 1;
> > 
> > whatever that is, it is wrong
[...]
> 
> As for pkt->pts, certain codecs, require that it be greater than the
> value of dts, hence it's set to pkt->dts+1, except when using b-frames
> in which case it's not modified.

all codecs require pts >= dts, that follows from the definition,
dts being the time when a frame is decoded, pts the one when it is
presented to the user (decoding must happen before presentation)

> 
> >> +int ff_concatgen_read_seek(AVFormatContext *s,
> >> +                           int stream_index,
> >> +                           int64_t pts,
> >> +                           int flags)
> >> +{
(Continue reading)

Permalink | Reply | 3 comments |

Gmane