[soc]: r2021 - in eac3: ac3dec.h ac3dec_data.c ac3dec_data.h eac3dec.c
Author: bwolowiec Date: Mon Mar 17 23:21:11 2008 New Revision: 2021 Log: added support for dependent stream channel map Modified: eac3/ac3dec.h eac3/ac3dec_data.c eac3/ac3dec_data.h eac3/eac3dec.c Modified: eac3/ac3dec.h ============================================================================== --- eac3/ac3dec.h (original) +++ eac3/ac3dec.h Mon Mar 17 23:21:11 2008 @@ -77,6 +77,7 @@ typedef struct AC3DecodeContext { int num_blocks; ///< Number of audio blocks int channel_mode; ///< Channel mode (acmod) int lfe_on; ///< Low frequency effect channel on (lfeon) + int channel_map; ///< Custom channel map int bitstream_id; ///< Bit stream identification (bsid) int center_mix_level; ///< Center mix level index int surround_mix_level; ///< Surround mix level index Modified: eac3/ac3dec_data.c ============================================================================== --- eac3/ac3dec_data.c (original) +++ eac3/ac3dec_data.c Mon Mar 17 23:21:11 2008 @@ -1105,6 +1105,23 @@ const uint8_t ff_eac3_frm_expstr[32][6] }; /** + * Default channel map for a dependent substream defined by acmod + */ +const uint16_t ff_eac3_default_chmap[8] = { + AC3_CHMAP_LEFT | AC3_CHMAP_RIGHT, // FIXME Ch1+Ch2 + AC3_CHMAP_CENTRE, + AC3_CHMAP_LEFT | AC3_CHMAP_RIGHT, + AC3_CHMAP_LEFT | AC3_CHMAP_CENTRE | AC3_CHMAP_RIGHT, + AC3_CHMAP_LEFT | AC3_CHMAP_RIGHT | AC3_CHMAP_CENTRE_SURROUND, + AC3_CHMAP_LEFT | AC3_CHMAP_CENTRE | AC3_CHMAP_RIGHT | + AC3_CHMAP_CENTRE_SURROUND, + AC3_CHMAP_LEFT | AC3_CHMAP_RIGHT | AC3_CHMAP_LEFT_SURROUND | + AC3_CHMAP_RIGHT_SURROUND, + AC3_CHMAP_LEFT | AC3_CHMAP_CENTRE | AC3_CHMAP_RIGHT | + AC3_CHMAP_LEFT_SURROUND | AC3_CHMAP_RIGHT_SURROUND +}; + +/** * Table E2.16 Default Coupling Banding Structure */ const uint8_t ff_eac3_default_cpl_band_struct[18] = Modified: eac3/ac3dec_data.h ============================================================================== --- eac3/ac3dec_data.h (original) +++ eac3/ac3dec_data.h Mon Mar 17 23:21:11 2008 @@ -37,4 +37,18 @@ extern const uint8_t ff_eac3_defecplbnds extern const uint8_t ff_ac3_rematrix_band_tab[5]; +extern const uint16_t ff_eac3_default_chmap[8]; + +/** Custom channel map locations bitmask + * Other channels described in documentation: Lc/Rc pair, Lrs/Rrs pair, Ts, Lsd/Rsd pair, Lw/Rw pair, Lvh/Rvh pair, Cvh, Reserved, LFE2 */ +enum { + AC3_CHMAP_LEFT = 1<<0, + AC3_CHMAP_CENTRE = 1<<1, + AC3_CHMAP_RIGHT = 1<<2, + AC3_CHMAP_LEFT_SURROUND = 1<<3, + AC3_CHMAP_RIGHT_SURROUND = 1<<4, + AC3_CHMAP_CENTRE_SURROUND = 1<<7, + AC3_CHMAP_LFE = 1<<15 +}; + #endif /* FFMPEG_AC3DEC_DATA_H */ Modified: eac3/eac3dec.c ============================================================================== --- eac3/eac3dec.c (original) +++ eac3/eac3dec.c Mon Mar 17 23:21:11 2008 @@ -331,9 +331,12 @@ static int parse_bsi(AC3DecodeContext *s /* dependent stream channel map */ if (s->stream_type == EAC3_STREAM_TYPE_DEPENDENT) { if (get_bits1(gbc)) { - skip_bits(gbc, 16); // skip custom channel map + s->channel_map = get_bits(gbc, 16); //custom channel map } else { - //TODO default channel map based on acmod and lfeon + //default channel map based on acmod and lfeon + s->channel_map = ff_eac3_default_chmap[s->channel_mode]; + if(s->lfe_on) + s->channel_map |= AC3_CHMAP_LFE; } } #endif
Currently, I'm trying to make changes which allow opening files with more channels than 5.1. Now the decoder is written in a way, which allows ac3_decode_frame to decode one frame, but additional channels in E-AC3 are sent in separate frames. Due to this, I have a question, how to solve it: - ac3_decode_frame function should decode many frames to get complete information about outstream, or - ac3_decode_frame function should return outstream data when it is called for the first frame from the next data package? -- Bartlomiej Wolowiec
On Tue, Mar 18, 2008 at 12:14:19AM +0100, Bartlomiej Wolowiec wrote:
Currently, I'm trying to make changes which allow opening files with more channels than 5.1. Now the decoder is written in a way, which allows ac3_decode_frame to decode one frame, but additional channels in E-AC3 are sent in separate frames. Due to this, I have a question, how to solve it: - ac3_decode_frame function should decode many frames to get complete information about outstream, or - ac3_decode_frame function should return outstream data when it is called for the first frame from the next data package?
The (E)AC-3 AVParser should split the (E)AC-3 stream so that the chunks of data send to the decoder (ac3_decode_frame) are complete and contain all channels. So ac3_decode_frame() would receive as input several "ac3 frames" if some channels are stored in future frames. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Good people do not need laws to tell them to act responsibly, while bad people will find a way around the laws. -- Plato
On wtorek, 18 marca 2008, Michael Niedermayer wrote:
The (E)AC-3 AVParser should split the (E)AC-3 stream so that the chunks of data send to the decoder (ac3_decode_frame) are complete and contain all channels. So ac3_decode_frame() would receive as input several "ac3 frames" if some channels are stored in future frames.
Hi, Recently I had some free time, so I've tried to work on the parser. I encountered one problem: ff_aac_ac3_parse function in aac_ac3_parser.c needs to know stream_type. As an interface to read data about the stream it uses sync pointer from AACAC3ParseContext, but this function returns only few values from the stream. Due to this, I have a question: how should I get stream_type? I have three ideas: - add argument to sync, AAC parser will need some changes then, - change sync, so that it won't use long list of arguments but a new defined structure (in the future there will be need to get substream_id...), or - read necessary bits directly from aac3_ac3_parse Maybe you have different ideas? -- Bartlomiej Wolowiec
On Thu, Mar 20, 2008 at 07:38:06PM +0100, Bartlomiej Wolowiec wrote:
On wtorek, 18 marca 2008, Michael Niedermayer wrote:
The (E)AC-3 AVParser should split the (E)AC-3 stream so that the chunks of data send to the decoder (ac3_decode_frame) are complete and contain all channels. So ac3_decode_frame() would receive as input several "ac3 frames" if some channels are stored in future frames.
Hi, Recently I had some free time, so I've tried to work on the parser. I encountered one problem: ff_aac_ac3_parse function in aac_ac3_parser.c needs to know stream_type. As an interface to read data about the stream it uses
What stream_type ? I dont understand you. ac3_sync() could just return the sum of the len of both frames (so all channels are available) or am i missing something? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
On czwartek, 20 marca 2008, Michael Niedermayer wrote:
What stream_type ? I dont understand you. ac3_sync() could just return the sum of the len of both frames (so all channels are available) or am i missing something? Oops, you're right, I haven't noticed what ac3_sync() returns... -- Bartlomiej Wolowiec
On czwartek, 20 marca 2008, Michael Niedermayer wrote:
On Thu, Mar 20, 2008 at 07:38:06PM +0100, Bartlomiej Wolowiec wrote:
On wtorek, 18 marca 2008, Michael Niedermayer wrote:
The (E)AC-3 AVParser should split the (E)AC-3 stream so that the chunks of data send to the decoder (ac3_decode_frame) are complete and contain all channels. So ac3_decode_frame() would receive as input several "ac3 frames" if some channels are stored in future frames.
Hi, Recently I had some free time, so I've tried to work on the parser. I encountered one problem: ff_aac_ac3_parse function in aac_ac3_parser.c needs to know stream_type. As an interface to read data about the stream it uses
What stream_type ? I dont understand you. ac3_sync() could just return the sum of the len of both frames (so all channels are available) or am i missing something?
No, it isn't that simple: ac3_sync is given a buffer, which has only header_size bytes. So it isn't able to read headers of following frames and it doesn't know if they're an extension of current frame.... -- Bartlomiej Wolowiec
Bartlomiej Wolowiec wrote:
On czwartek, 20 marca 2008, Michael Niedermayer wrote:
On Thu, Mar 20, 2008 at 07:38:06PM +0100, Bartlomiej Wolowiec wrote:
On wtorek, 18 marca 2008, Michael Niedermayer wrote:
The (E)AC-3 AVParser should split the (E)AC-3 stream so that the chunks of data send to the decoder (ac3_decode_frame) are complete and contain all channels. So ac3_decode_frame() would receive as input several "ac3 frames" if some channels are stored in future frames. Hi, Recently I had some free time, so I've tried to work on the parser. I encountered one problem: ff_aac_ac3_parse function in aac_ac3_parser.c needs to know stream_type. As an interface to read data about the stream it uses What stream_type ? I dont understand you. ac3_sync() could just return the sum of the len of both frames (so all channels are available) or am i missing something?
No, it isn't that simple: ac3_sync is given a buffer, which has only header_size bytes. So it isn't able to read headers of following frames and it doesn't know if they're an extension of current frame....
Exactly. That's probably going to require reworking the parser a bit. I can look through and give some suggestions if you want. But feel free to propose any changes to the parser that would fit what you need to do. -Justin
On czwartek, 20 marca 2008, Justin Ruggles wrote:
Exactly. That's probably going to require reworking the parser a bit. I can look through and give some suggestions if you want. But feel free to propose any changes to the parser that would fit what you need to do.
I showed some ideas in mail few hours ago. Could you look at them and tell me which solution will be the best? -- Bartlomiej Wolowiec
Bartlomiej Wolowiec wrote:
On wtorek, 18 marca 2008, Michael Niedermayer wrote:
The (E)AC-3 AVParser should split the (E)AC-3 stream so that the chunks of data send to the decoder (ac3_decode_frame) are complete and contain all channels. So ac3_decode_frame() would receive as input several "ac3 frames" if some channels are stored in future frames.
Hi, Recently I had some free time, so I've tried to work on the parser. I encountered one problem: ff_aac_ac3_parse function in aac_ac3_parser.c needs to know stream_type. As an interface to read data about the stream it uses sync pointer from AACAC3ParseContext, but this function returns only few values from the stream. Due to this, I have a question: how should I get stream_type? I have three ideas: - add argument to sync, AAC parser will need some changes then, - change sync, so that it won't use long list of arguments but a new defined structure (in the future there will be need to get substream_id...), or
Either of these 2 would be fine with me.
- read necessary bits directly from aac3_ac3_parse
I don't like this one
Maybe you have different ideas?
For the issue you're concerned about, what you have proposed will work fine. I can only make assumptions about how these changes fit into the rest of your solution. But you seem to be on the right track. -Justin
On piątek, 21 marca 2008, Justin Ruggles wrote:
- add argument to sync, AAC parser will need some changes then, - change sync, so that it won't use long list of arguments but a new defined structure (in the future there will be need to get substream_id...), or
Either of these 2 would be fine with me.
Here is my idea of changing ac3 parser, so that it will send in one package to decoder all frames concerning one part of time. It works like that: it buffers all frames until next independent frame. The problem is that the last frame never leaves the buffer. Any ideas how to solve it? I send new ffmpeg.patch (patch of patch isn't easily readable, but it's easy to tell if the change concerns >5.1 or not) -- Bartlomiej Wolowiec
ooops, my last patch doesn't work... I am fixing it right now... -- Bartlomiej Wolowiec
while removing redundant parts of the code I've removed quite important part... I enclose fixed version -- Bartlomiej Wolowiec
On Sat, Mar 22, 2008 at 12:19:37AM +0100, Bartlomiej Wolowiec wrote:
while removing redundant parts of the code I've removed quite important part... I enclose fixed version
This contains several unrelated changes. First the the addition of AACAC3HeaderInfo, IMHO you should simply pass a pointer to AACAC3ParseContext into sync(). This should be in a seperate patch and can likely hit ffmpeg svn quickly. Then there is the change which adds a bap_tab parameter this is useless as its always ff_ac3_bap_tab. Then theres the export of num_blocks, that as well belongs into a seperate patch and ill like to hear 1 sentance why it is needed. But i guess this as well can hit svn quickly The rest ill review when above is seperated out. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
On sobota, 22 marca 2008, Michael Niedermayer wrote:
This contains several unrelated changes.
First the the addition of AACAC3HeaderInfo, IMHO you should simply pass a pointer to AACAC3ParseContext into sync(). This should be in a seperate patch and can likely hit ffmpeg svn quickly.
As I wrote, this is new version of ffmpeg.patch from soc/eac3 repository, because without eac3 this makes no sense. From this file derived other updates (e.g. bap_tab is sometimes ff_ac3_bap_tab and sometimes ff_eac3_hebap_tab, and other changes concerning eac3). But maybe is better to send to svn ffmpeg this patch and then start to update the rest, so i enclose separate patch. -- Bartlomiej Wolowiec
On Sat, Mar 22, 2008 at 11:52:31AM +0100, Bartlomiej Wolowiec wrote:
On sobota, 22 marca 2008, Michael Niedermayer wrote:
This contains several unrelated changes.
First the the addition of AACAC3HeaderInfo, IMHO you should simply pass a pointer to AACAC3ParseContext into sync(). This should be in a seperate patch and can likely hit ffmpeg svn quickly.
As I wrote, this is new version of ffmpeg.patch from soc/eac3 repository, because without eac3 this makes no sense. From this file derived other updates (e.g. bap_tab is sometimes ff_ac3_bap_tab and sometimes ff_eac3_hebap_tab, and other changes concerning eac3). But maybe is better to send to svn ffmpeg this patch
Yes i like to move things into ffmpeg svn where possible.
and then start to update the rest, so i enclose separate patch.
This patch is still full of unrelated changes. First send a patch doing just the change of the sync() arguments->AACAC3ParseContext and nothing else. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
On sobota, 22 marca 2008, Michael Niedermayer wrote:
First send a patch doing just the change of the sync() arguments->AACAC3ParseContext and nothing else.
Ok, I enclose patch. -- Bartlomiej Wolowiec
On Sat, Mar 22, 2008 at 06:23:11PM +0100, Bartlomiej Wolowiec wrote:
On sobota, 22 marca 2008, Michael Niedermayer wrote:
First send a patch doing just the change of the sync() arguments->AACAC3ParseContext and nothing else.
Ok, I enclose patch.
ok, can be commited to ffmpeg-svn if it works [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML.
On sobota, 22 marca 2008, Michael Niedermayer wrote:
Ok, I enclose patch.
ok, can be commited to ffmpeg-svn if it works
Ok, patch worked good in all my tests, who should commit it to ffmpeg-svn? -- Bartlomiej Wolowiec
Bartlomiej Wolowiec wrote:
On sobota, 22 marca 2008, Michael Niedermayer wrote:
Ok, I enclose patch. ok, can be commited to ffmpeg-svn if it works
Ok, patch worked good in all my tests, who should commit it to ffmpeg-svn?
I believe you should have write access to ffmpeg-svn as well. If not then let me know and I will commit it. -Justin
On niedziela, 23 marca 2008, Justin Ruggles wrote:
I believe you should have write access to ffmpeg-svn as well. If not then let me know and I will commit it.
Unfortunately, I think that I have no access to main FFmpeg repository... -- Bartlomiej Wolowiec
Bartlomiej Wolowiec wrote:
On niedziela, 23 marca 2008, Justin Ruggles wrote:
I believe you should have write access to ffmpeg-svn as well. If not then let me know and I will commit it.
Unfortunately, I think that I have no access to main FFmpeg repository...
ok. I thought soc committers also had access to ffmpeg... patch applied.
I send next part of update. Now patch allows using stream type in parser. -- Bartlomiej Wolowiec
On Sun, Mar 23, 2008 at 06:39:55PM +0100, Bartlomiej Wolowiec wrote:
I send next part of update. Now patch allows using stream type in parser.
looks ok if justin is ok with it. also ask diego if you want ffmpeg svn write [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct awnser.
On Sun, Mar 23, 2008 at 08:05:49PM +0100, Michael Niedermayer wrote:
On Sun, Mar 23, 2008 at 06:39:55PM +0100, Bartlomiej Wolowiec wrote:
I send next part of update. Now patch allows using stream type in parser.
looks ok if justin is ok with it. also ask diego if you want ffmpeg svn write
Account created. Diego
Bartlomiej Wolowiec wrote:
I send next part of update. Now patch allows using stream type in parser.
Maybe you should also go ahead and add an enum for stream types to ac3.h. Besides that, patch looks fine to me. -Justin
On niedziela, 23 marca 2008, Justin Ruggles wrote:
Maybe you should also go ahead and add an enum for stream types to ac3.h. Besides that, patch looks fine to me.
ok, this is patch with added enum EAC3StreamType. I also wrote to Diego about access to the repository. -- Bartlomiej Wolowiec
Bartlomiej Wolowiec wrote:
On niedziela, 23 marca 2008, Justin Ruggles wrote:
Maybe you should also go ahead and add an enum for stream types to ac3.h. Besides that, patch looks fine to me.
ok, this is patch with added enum EAC3StreamType. I also wrote to Diego about access to the repository.
Looks good. You can apply it. -Justin
On poniedziałek, 24 marca 2008, Justin Ruggles wrote:
Looks good. You can apply it.
patch applied... -- Bartlomiej Wolowiec
This is last part of patch written earlier:
Here is my idea of changing ac3 parser, so that it will send in one package to decoder all frames concerning one part of time. It works like that: it buffers all frames until next independent frame. The problem is that the last frame never leaves the buffer. Any ideas how to solve it?
-- Bartlomiej Wolowiec
On Wed, Mar 26, 2008 at 01:33:28PM +0100, Bartlomiej Wolowiec wrote:
This is last part of patch written earlier:
Here is my idea of changing ac3 parser, so that it will send in one package to decoder all frames concerning one part of time. It works like that: it buffers all frames until next independent frame. The problem is that the last frame never leaves the buffer. Any ideas how to solve it?
first, after reviewing our ac3 parser i must say i do not like it as it always copies the data even if completely unneeded. Our mp3 parser while maybe being a mess does not. This is not strictly related to the patch. But i will not tolerate any further unneeded copying being added, even less so if it adds a 1 packet delay (for normal AC3/AAC). [...]
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12599) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -23,6 +23,7 @@ #include "parser.h" #include "aac_ac3_parser.h"
+// TODO check s->inbuf_tab size int ff_aac_ac3_parse(AVCodecParserContext *s1, AVCodecContext *avctx, const uint8_t **poutbuf, int *poutbuf_size, @@ -36,6 +37,13 @@ *poutbuf_size = 0;
buf_ptr = buf; + + if(s->inbuf_ptr < s->inbuf) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf_tab, s->inbuf, s->header_size); + s->inbuf = s->inbuf_tab; + s->inbuf_ptr = s->inbuf_tab + s->header_size; + }
The if condition just looks invalid. I know what you are doing here but at least with the current variable names this makes no sense to a reader at all. Also i doubt renaming inbuf is the cleanest solution ...
while (buf_size > 0) { int size_needed= s->frame_size ? s->frame_size : s->header_size; len = s->inbuf_ptr - s->inbuf;
@@ -56,7 +64,14 @@ memmove(s->inbuf, s->inbuf + 1, s->header_size - 1); s->inbuf_ptr--; } else { - s->frame_size = len; + if(!s->stream_type) { + if(s->inbuf != s->inbuf_tab) { + *poutbuf = s->inbuf_tab; + *poutbuf_size = s->inbuf - s->inbuf_tab; + s->inbuf_ptr = s->inbuf_tab; + s->frame_size = 0; + break; + }
this is VERY hackish, stream_type has from the point of view of a generic (AAC+AC3+EAC3) parser no relation to the number of frames over which channels are split.
/* update codec info */ avctx->sample_rate = s->sample_rate; /* allow downmixing to stereo (or mono for AC3) */ @@ -71,15 +86,18 @@ } avctx->bit_rate = s->bit_rate; avctx->frame_size = s->samples; + } else { + //TODO update bit_rate + avctx->frame_size += s->samples; + } + s->frame_size = len; } } } else {
if(s->inbuf_ptr - s->inbuf == s->frame_size){ - *poutbuf = s->inbuf; - *poutbuf_size = s->frame_size; + s->inbuf += s->frame_size; s->inbuf_ptr = s->inbuf; s->frame_size = 0; - break; }
this causes additional copying and delay i think for normal AC3 & AAC [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato
On środa, 26 marca 2008, Michael Niedermayer wrote:
while (buf_size > 0) { int size_needed= s->frame_size ? s->frame_size : s->header_size; len = s->inbuf_ptr - s->inbuf;
@@ -56,7 +64,14 @@ memmove(s->inbuf, s->inbuf + 1, s->header_size - 1); s->inbuf_ptr--; } else { - s->frame_size = len; + if(!s->stream_type) { + if(s->inbuf != s->inbuf_tab) { + *poutbuf = s->inbuf_tab; + *poutbuf_size = s->inbuf - s->inbuf_tab; + s->inbuf_ptr = s->inbuf_tab; + s->frame_size = 0; + break; + }
this is VERY hackish, stream_type has from the point of view of a generic (AAC+AC3+EAC3) parser no relation to the number of frames over which channels are split.
/* update codec info */ avctx->sample_rate = s->sample_rate; /* allow downmixing to stereo (or mono for AC3) */ @@ -71,15 +86,18 @@ } avctx->bit_rate = s->bit_rate; avctx->frame_size = s->samples; + } else { + //TODO update bit_rate + avctx->frame_size += s->samples; + } + s->frame_size = len; } } } else {
if(s->inbuf_ptr - s->inbuf == s->frame_size){ - *poutbuf = s->inbuf; - *poutbuf_size = s->frame_size; + s->inbuf += s->frame_size; s->inbuf_ptr = s->inbuf; s->frame_size = 0; - break; }
this causes additional copying and delay i think for normal AC3 & AAC
Hi, Maybe this: if(eac3 stream){ // new code }else{ // existing code } will be a good solution for these problems? -- Bartlomiej Wolowiec
On Wed, Mar 26, 2008 at 07:42:59PM +0100, Bartlomiej Wolowiec wrote:
On środa, 26 marca 2008, Michael Niedermayer wrote:
while (buf_size > 0) { int size_needed= s->frame_size ? s->frame_size : s->header_size; len = s->inbuf_ptr - s->inbuf;
@@ -56,7 +64,14 @@ memmove(s->inbuf, s->inbuf + 1, s->header_size - 1); s->inbuf_ptr--; } else { - s->frame_size = len; + if(!s->stream_type) { + if(s->inbuf != s->inbuf_tab) { + *poutbuf = s->inbuf_tab; + *poutbuf_size = s->inbuf - s->inbuf_tab; + s->inbuf_ptr = s->inbuf_tab; + s->frame_size = 0; + break; + }
this is VERY hackish, stream_type has from the point of view of a generic (AAC+AC3+EAC3) parser no relation to the number of frames over which channels are split.
/* update codec info */ avctx->sample_rate = s->sample_rate; /* allow downmixing to stereo (or mono for AC3) */ @@ -71,15 +86,18 @@ } avctx->bit_rate = s->bit_rate; avctx->frame_size = s->samples; + } else { + //TODO update bit_rate + avctx->frame_size += s->samples; + } + s->frame_size = len; } } } else {
if(s->inbuf_ptr - s->inbuf == s->frame_size){ - *poutbuf = s->inbuf; - *poutbuf_size = s->frame_size; + s->inbuf += s->frame_size; s->inbuf_ptr = s->inbuf; s->frame_size = 0; - break; }
this causes additional copying and delay i think for normal AC3 & AAC
Hi, Maybe this: if(eac3 stream){ // new code }else{ // existing code } will be a good solution for these problems?
I do not like generic code full of If(codecA) else The sync function should return if it has a complete frame or if (maybe) not. That is flags= FRAME_START | FRAME_END; You do this in stream_type but this name is totally wrong this is not a stream_type. It is not even constant for a stream. NEVER even think of using a name from the a52* spec, the people who wrote it are idiots. Its a flag indicating the start/end of a frame. And IMHO it should be returned more directly than as a context variable but thats rather minor the name is a major issue. It totally confused me until looked it up in the spec. Code should be understandable without looking things up in the specs. Also the "stream_type" has the wrong type (should be enum) and the types used in AAC-AC3 code must be generic types not *AC3 specific types in headers which arent #included. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
2008/3/26, Michael Niedermayer <michaelni@gmx.at>:
I do not like generic code full of If(codecA) else
The sync function should return if it has a complete frame or if (maybe) not. That is
flags= FRAME_START | FRAME_END;
You do this in stream_type but this name is totally wrong this is not a stream_type. It is not even constant for a stream. NEVER even think of using a name from the a52* spec, the people who wrote it are idiots. Its a flag indicating the start/end of a frame. And IMHO it should be returned more directly than as a context variable but thats rather minor the name is a major issue. It totally confused me until looked it up in the spec.
Code should be understandable without looking things up in the specs.
Also the "stream_type" has the wrong type (should be enum) and the types used in AAC-AC3 code must be generic types not *AC3 specific types in headers which arent #included.
Ok, what do you think about such a plan: 1. enum AACAC3FrameFlag{ FRAME_COMPLETE, FRAME_START, FRAME_CONTINUATION }; removal of stream_type z AACAC3ParseContext, change of int (*sync)(struct AACAC3ParseContext *hdr_info) to int (*sync)(struct AACAC3ParseContext *hdr_info, AACAC3FrameFlag *flag) 2.change of stream_type to frame_type 3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag. -- Bartlomiej Wolowiec
On Thu, Mar 27, 2008 at 10:28:51PM +0100, Bartlomiej Wolowiec wrote:
2008/3/26, Michael Niedermayer <michaelni@gmx.at>:
I do not like generic code full of If(codecA) else
The sync function should return if it has a complete frame or if (maybe) not. That is
flags= FRAME_START | FRAME_END;
You do this in stream_type but this name is totally wrong this is not a stream_type. It is not even constant for a stream. NEVER even think of using a name from the a52* spec, the people who wrote it are idiots. Its a flag indicating the start/end of a frame. And IMHO it should be returned more directly than as a context variable but thats rather minor the name is a major issue. It totally confused me until looked it up in the spec.
Code should be understandable without looking things up in the specs.
Also the "stream_type" has the wrong type (should be enum) and the types used in AAC-AC3 code must be generic types not *AC3 specific types in headers which arent #included.
Ok, what do you think about such a plan:
1. enum AACAC3FrameFlag{ FRAME_COMPLETE, FRAME_START, FRAME_CONTINUATION }; removal of stream_type z AACAC3ParseContext, change of int (*sync)(struct AACAC3ParseContext *hdr_info) to int (*sync)(struct AACAC3ParseContext *hdr_info, AACAC3FrameFlag *flag)
2.change of stream_type to frame_type
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
On czwartek, 27 marca 2008, Michael Niedermayer wrote:
1. enum AACAC3FrameFlag{ FRAME_COMPLETE, FRAME_START, FRAME_CONTINUATION }; removal of stream_type z AACAC3ParseContext, change of int (*sync)(struct AACAC3ParseContext *hdr_info) to int (*sync)(struct AACAC3ParseContext *hdr_info, AACAC3FrameFlag *flag)
2.change of stream_type to frame_type
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
ok
this is the first patch: "removal of stream_type in AACAC3ParseContext and adding AACAC3FrameFlag" -- Bartlomiej Wolowiec
On Fri, Mar 28, 2008 at 06:32:38PM +0100, Bartlomiej Wolowiec wrote:
On czwartek, 27 marca 2008, Michael Niedermayer wrote:
1. enum AACAC3FrameFlag{ FRAME_COMPLETE, FRAME_START, FRAME_CONTINUATION }; removal of stream_type z AACAC3ParseContext, change of int (*sync)(struct AACAC3ParseContext *hdr_info) to int (*sync)(struct AACAC3ParseContext *hdr_info, AACAC3FrameFlag *flag)
2.change of stream_type to frame_type
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
ok
this is the first patch: "removal of stream_type in AACAC3ParseContext and adding AACAC3FrameFlag" [...]
+typedef enum{ + /** finishes portions of data */ + FRAME_COMPLETE, + /** starts portions of data + * (if there are any frames in the buffer they're send) */ + FRAME_START, + /** continues portions of data (adds them to buffer) */ + FRAME_CONTINUATION +}AACAC3FrameFlag;
[...]
-static int ac3_sync(AACAC3ParseContext *hdr_info) +static int ac3_sync(AACAC3ParseContext *hdr_info, AACAC3FrameFlag *flag) { int err; AC3HeaderInfo hdr; @@ -137,6 +137,9 @@ hdr_info->bit_rate = hdr.bit_rate; hdr_info->channels = hdr.channels; hdr_info->samples = AC3_FRAME_SIZE; + + *flag = (hdr.stream_type == EAC3_STREAM_TYPE_INDEPENDENT)? + FRAME_START:FRAME_CONTINUATION; return hdr.frame_size; }
There are 3 types in EAC3 Type 0: always start a frame and may or may not have further parts following Type 1: never start a frame and may or may not have further parts following Type 2: always start a frame and never have further parts following That is |T0 | T0 T1 | T0 T1 T1 ... T1 | T2 | are all and the only valid combinations accoriding to my spec I cannot map this to your code at all [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact
On piątek, 28 marca 2008, Michael Niedermayer wrote:
There are 3 types in EAC3 Type 0: always start a frame and may or may not have further parts following Type 1: never start a frame and may or may not have further parts following Type 2: always start a frame and never have further parts following
That is
|T0 | T0 T1 | T0 T1 T1 ... T1 | T2 |
are all and the only valid combinations accoriding to my spec
I cannot map this to your code at all
[...]
You're right I forgot about frames EAC3_STREAM_TYPE_AC3_CONVERT type, which should be given FRAME_COMPLETE flag -- Bartlomiej Wolowiec
On Fri, Mar 28, 2008 at 08:11:32PM +0100, Bartlomiej Wolowiec wrote:
On piątek, 28 marca 2008, Michael Niedermayer wrote:
There are 3 types in EAC3 Type 0: always start a frame and may or may not have further parts following Type 1: never start a frame and may or may not have further parts following Type 2: always start a frame and never have further parts following
That is
|T0 | T0 T1 | T0 T1 T1 ... T1 | T2 |
are all and the only valid combinations accoriding to my spec
I cannot map this to your code at all
[...]
You're right I forgot about frames EAC3_STREAM_TYPE_AC3_CONVERT type, which should be given FRAME_COMPLETE flag
-- Bartlomiej Wolowiec
[...]
+typedef enum{ + /** finishes portions of data */ + FRAME_COMPLETE, + /** starts portions of data + * (if there are any frames in the buffer they're send) */ + FRAME_START, + /** continues portions of data (adds them to buffer) */ + FRAME_CONTINUATION +}AACAC3FrameFlag;
Please use: Complete frame, ends previous frame Frame start, ends previous frame Part of the previous frame And patch ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle
On piątek, 28 marca 2008, Michael Niedermayer wrote:
+typedef enum{ + /** finishes portions of data */ + FRAME_COMPLETE, + /** starts portions of data + * (if there are any frames in the buffer they're send) */ + FRAME_START, + /** continues portions of data (adds them to buffer) */ + FRAME_CONTINUATION +}AACAC3FrameFlag;
Please use: Complete frame, ends previous frame Frame start, ends previous frame Part of the previous frame
And patch ok
patch applied. -- Bartlomiej Wolowiec
On czwartek, 27 marca 2008, Michael Niedermayer wrote:
You do this in stream_type but this name is totally wrong this is not a stream_type. It is not even constant for a stream. NEVER even think of using a name from the a52* spec, the people who wrote it are idiots. Its a flag indicating the start/end of a frame. And IMHO it should be returned more directly than as a context variable but thats rather minor the name is a major issue. It totally confused me until looked it up in the spec. [...] 2.change of stream_type to frame_type
According to your suggestion, I changed name of stream type to frame type. Should correction of type of frame_type from uint8_t to EAC3FrameType be in separate patch? -- Bartlomiej Wolowiec
On Fri, Mar 28, 2008 at 09:20:05PM +0100, Bartlomiej Wolowiec wrote:
On czwartek, 27 marca 2008, Michael Niedermayer wrote:
You do this in stream_type but this name is totally wrong this is not a stream_type. It is not even constant for a stream. NEVER even think of using a name from the a52* spec, the people who wrote it are idiots. Its a flag indicating the start/end of a frame. And IMHO it should be returned more directly than as a context variable but thats rather minor the name is a major issue. It totally confused me until looked it up in the spec. [...] 2.change of stream_type to frame_type
According to your suggestion, I changed name of stream type to frame type. Should correction of type of frame_type from uint8_t to EAC3FrameType be in separate patch?
Index: libavcodec/ac3dec.c =================================================================== --- libavcodec/ac3dec.c (wersja 12621) +++ libavcodec/ac3dec.c (kopia robocza) @@ -1169,7 +1169,7 @@ case AC3_PARSE_ERROR_FRAME_SIZE: av_log(avctx, AV_LOG_ERROR, "invalid frame size\n"); break; - case AC3_PARSE_ERROR_STREAM_TYPE: + case AC3_PARSE_ERROR_FRAME_TYPE: av_log(avctx, AV_LOG_ERROR, "invalid stream type\n"); ^^^^^^
yes but thats minor ... [...] this would confuse people remainder of the patch looks ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On piątek, 28 marca 2008, Michael Niedermayer wrote:
Index: libavcodec/ac3dec.c =================================================================== --- libavcodec/ac3dec.c (wersja 12621) +++ libavcodec/ac3dec.c (kopia robocza) @@ -1169,7 +1169,7 @@ case AC3_PARSE_ERROR_FRAME_SIZE: av_log(avctx, AV_LOG_ERROR, "invalid frame size\n"); break; - case AC3_PARSE_ERROR_STREAM_TYPE: + case AC3_PARSE_ERROR_FRAME_TYPE: av_log(avctx, AV_LOG_ERROR, "invalid stream type\n");
^^^^^^ this would confuse people remainder of the patch looks ok
[...]
patch applied -- Bartlomiej Wolowiec
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
According to suggestions, I wrote new patch to AAC/AC3 parser. Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote:
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
According to suggestions, I wrote new patch to AAC/AC3 parser.
Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12623) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -29,35 +29,50 @@ const uint8_t *buf, int buf_size) { AACAC3ParseContext *s = s1->priv_data; - AACAC3FrameFlag frame_flag; const uint8_t *buf_ptr; int len;
*poutbuf = NULL; *poutbuf_size = 0;
+ if(s->frame_ptr == NULL) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf, s->frame_start, s->header_size); + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start + s->header_size; + }
Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*.
+ buf_ptr = buf; while (buf_size > 0) { int size_needed= s->frame_size ? s->frame_size : s->header_size;
- len = s->inbuf_ptr - s->inbuf; + len = s->frame_ptr - s->frame_start;
Variable renaming is cosmetic and should be in a seperate patch! [...]
@@ -43,6 +44,7 @@ int sample_rate; int bit_rate; int samples; + AACAC3FrameFlag frame_flag; } AACAC3ParseContext;
why? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
On sobota, 29 marca 2008, Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote:
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
According to suggestions, I wrote new patch to AAC/AC3 parser.
Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12623) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -29,35 +29,50 @@ const uint8_t *buf, int buf_size) { AACAC3ParseContext *s = s1->priv_data; - AACAC3FrameFlag frame_flag; const uint8_t *buf_ptr; int len;
*poutbuf = NULL; *poutbuf_size = 0;
+ if(s->frame_ptr == NULL) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf, s->frame_start, s->header_size); + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start + s->header_size; + }
Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*.
I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean?
@@ -43,6 +44,7 @@ int sample_rate; int bit_rate; int samples; + AACAC3FrameFlag frame_flag; } AACAC3ParseContext;
why?
Because of the code below, I need to know if the frame should be send instantly or stay in the buffer and wait for next frames: + if(s->frame_flag == FRAME_COMPLETE) { *poutbuf = s->inbuf; *poutbuf_size = s->frame_size; - s->inbuf_ptr = s->inbuf; - s->frame_size = 0; + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start; break; -- Bartlomiej Wolowiec
On Sat, Mar 29, 2008 at 10:04:30PM +0100, Bartlomiej Wolowiec wrote:
On sobota, 29 marca 2008, Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote:
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag.
According to suggestions, I wrote new patch to AAC/AC3 parser.
Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12623) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -29,35 +29,50 @@ const uint8_t *buf, int buf_size) { AACAC3ParseContext *s = s1->priv_data; - AACAC3FrameFlag frame_flag; const uint8_t *buf_ptr; int len;
*poutbuf = NULL; *poutbuf_size = 0;
+ if(s->frame_ptr == NULL) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf, s->frame_start, s->header_size); + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start + s->header_size; + }
Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*.
I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean?
I meant something that didnt exist. Sorry! Either way after looking at the parser code again and reading the spec again i think there are a few problems. First could you explain me how these 7+ch EAC-3 streams are stored in MPEG? Second how are timestamps associated with them if they are stored in a single ES stream instead of 2. Can timestamps be associated with dependant "frames" or not? (If we do not know this its not possible to implement 7+ch EAC-3) Either way, a parser as you implement it must at least return the correct index (which can be negative up to the header size) current suggested code returns something random. Also it requires at least an increase of AV_PARSER_PTS_NB. And this design requires that timestamps can not be associated with dependant "frames". If they can more changes or better a different design has to be choosen. besides, do we have sample files for 7+ch EAC3 ? Especially ones muxed in MPEG?
@@ -43,6 +44,7 @@ int sample_rate; int bit_rate; int samples; + AACAC3FrameFlag frame_flag; } AACAC3ParseContext;
why?
Because of the code below, I need to know if the frame should be send instantly or stay in the buffer and wait for next frames:
+ if(s->frame_flag == FRAME_COMPLETE) { *poutbuf = s->inbuf; *poutbuf_size = s->frame_size; - s->inbuf_ptr = s->inbuf; - s->frame_size = 0; + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start; break;
Hmm, well, ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 10:04:30PM +0100, Bartlomiej Wolowiec wrote:
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote:
3.change of ff_aac_ac3_parse to make it react correctly to result returned in flag. According to suggestions, I wrote new patch to AAC/AC3 parser.
Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12623) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -29,35 +29,50 @@ const uint8_t *buf, int buf_size) { AACAC3ParseContext *s = s1->priv_data; - AACAC3FrameFlag frame_flag; const uint8_t *buf_ptr; int len;
*poutbuf = NULL; *poutbuf_size = 0;
+ if(s->frame_ptr == NULL) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf, s->frame_start, s->header_size); + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start + s->header_size; + } Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*. I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside
On sobota, 29 marca 2008, Michael Niedermayer wrote: the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean?
I meant something that didnt exist. Sorry! Either way after looking at the parser code again and reading the spec again i think there are a few problems.
First could you explain me how these 7+ch EAC-3 streams are stored in MPEG? Second how are timestamps associated with them if they are stored in a single ES stream instead of 2. Can timestamps be associated with dependant "frames" or not? (If we do not know this its not possible to implement 7+ch EAC-3)
Either way, a parser as you implement it must at least return the correct index (which can be negative up to the header size) current suggested code returns something random. Also it requires at least an increase of AV_PARSER_PTS_NB. And this design requires that timestamps can not be associated with dependant "frames". If they can more changes or better a different design has to be choosen.
besides, do we have sample files for 7+ch EAC3 ? Especially ones muxed in MPEG?
We have 1 sample for 7.1 E-AC3. I can't find the original EVO that it came from, so I sent a message to the person who provided it originally. I'm sure madshi (eac3to author) still has it lying around somewhere as well... (are you subscribed here madshi?) -Justin
Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote:
> 3.change of ff_aac_ac3_parse to make it react correctly to result > returned in flag. According to suggestions, I wrote new patch to AAC/AC3 parser.
Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12623) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -29,35 +29,50 @@ const uint8_t *buf, int buf_size) { AACAC3ParseContext *s = s1->priv_data; - AACAC3FrameFlag frame_flag; const uint8_t *buf_ptr; int len;
*poutbuf = NULL; *poutbuf_size = 0;
+ if(s->frame_ptr == NULL) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf, s->frame_start, s->header_size); + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start + s->header_size; + } Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*. I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside
On sobota, 29 marca 2008, Michael Niedermayer wrote: the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean? I meant something that didnt exist. Sorry! Either way after looking at the parser code again and reading the spec again i
On Sat, Mar 29, 2008 at 10:04:30PM +0100, Bartlomiej Wolowiec wrote: think there are a few problems.
First could you explain me how these 7+ch EAC-3 streams are stored in MPEG? Second how are timestamps associated with them if they are stored in a single ES stream instead of 2. Can timestamps be associated with dependant "frames" or not? (If we do not know this its not possible to implement 7+ch EAC-3)
Either way, a parser as you implement it must at least return the correct index (which can be negative up to the header size) current suggested code returns something random. Also it requires at least an increase of AV_PARSER_PTS_NB. And this design requires that timestamps can not be associated with dependant "frames". If they can more changes or better a different design has to be choosen.
besides, do we have sample files for 7+ch EAC3 ? Especially ones muxed in MPEG?
We have 1 sample for 7.1 E-AC3. I can't find the original EVO that it came from, so I sent a message to the person who provided it originally. I'm sure madshi (eac3to author) still has it lying around somewhere as well... (are you subscribed here madshi?)
correction. We have 2 samples. I uploaded a different EVO sample to http://samples.mplayerhq.hu/A-codecs/AC3/eac3/7_pt_1_sample.evo md5sum: 5ef254a48ebfeecff22b02b7167e5987 7_pt_1_sample.evo -Justin
On Sat, Mar 29, 2008 at 08:16:24PM -0400, Justin Ruggles wrote:
Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote:
>> 3.change of ff_aac_ac3_parse to make it react correctly to result >> returned in flag. According to suggestions, I wrote new patch to AAC/AC3 parser.
Current data about the stream are taken from frames FRAME_START and FRAME_COMPLETE. It will be improved in the next patch. -- Bartlomiej Wolowiec
Index: libavcodec/aac_ac3_parser.c =================================================================== --- libavcodec/aac_ac3_parser.c (wersja 12623) +++ libavcodec/aac_ac3_parser.c (kopia robocza) @@ -29,35 +29,50 @@ const uint8_t *buf, int buf_size) { AACAC3ParseContext *s = s1->priv_data; - AACAC3FrameFlag frame_flag; const uint8_t *buf_ptr; int len;
*poutbuf = NULL; *poutbuf_size = 0;
+ if(s->frame_ptr == NULL) { + //after sending package of data in the end there was one new header + memcpy(s->inbuf, s->frame_start, s->header_size); + s->frame_start = s->inbuf; + s->frame_ptr = s->frame_start + s->header_size; + } Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*. I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside
On sobota, 29 marca 2008, Michael Niedermayer wrote: the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean? I meant something that didnt exist. Sorry! Either way after looking at the parser code again and reading the spec again i
On Sat, Mar 29, 2008 at 10:04:30PM +0100, Bartlomiej Wolowiec wrote: think there are a few problems.
First could you explain me how these 7+ch EAC-3 streams are stored in MPEG? Second how are timestamps associated with them if they are stored in a single ES stream instead of 2. Can timestamps be associated with dependant "frames" or not? (If we do not know this its not possible to implement 7+ch EAC-3)
Either way, a parser as you implement it must at least return the correct index (which can be negative up to the header size) current suggested code returns something random. Also it requires at least an increase of AV_PARSER_PTS_NB. And this design requires that timestamps can not be associated with dependant "frames". If they can more changes or better a different design has to be choosen.
besides, do we have sample files for 7+ch EAC3 ? Especially ones muxed in MPEG?
We have 1 sample for 7.1 E-AC3. I can't find the original EVO that it came from, so I sent a message to the person who provided it originally. I'm sure madshi (eac3to author) still has it lying around somewhere as well... (are you subscribed here madshi?)
correction. We have 2 samples. I uploaded a different EVO sample to
http://samples.mplayerhq.hu/A-codecs/AC3/eac3/7_pt_1_sample.evo
Good news, Timestamps do only apply to the first of the 2 chunks, so we were on the right track. Returning the correct (possibly negative) index and changing AV_PARSER_PTS_NB should be correct ... My notes of the file being below for the ones bored ... av_read_packet stream=1, pts=5400, dts=5400, size=2013 av_read_frame_internal stream=1, pts=5400, dts=5400, size=620 <---5400 av_read_frame_internal stream=1, pts=8280, dts=8280, size=488 av_read_frame_internal stream=1, pts=11160, dts=11160, size=620 (5888) av_read_packet stream=1, pts=6360, dts=6360, size=2016 av_read_frame_internal stream=1, pts=14040, dts=14040, size=488 av_read_frame_internal stream=1, pts=6360, dts=6360, size=620 <---6360 av_read_frame_internal stream=1, pts=9240, dts=9240, size=488 av_read_frame_internal stream=1, pts=12120, dts=12120, size=620 (6840) av_read_packet stream=1, pts=7320, dts=7320, size=2016 85 av_read_frame_internal stream=1, pts=15000, dts=15000, size=488 av_read_frame_internal stream=1, pts=7320, dts=7320, size=620 <---7320 av_read_frame_internal stream=1, pts=10200, dts=10200, size=488 av_read_packet stream=1, pts=8280, dts=8280, size=2016 505 av_read_frame_internal stream=1, pts=13080, dts=13080, size=620 (7800) av_read_frame_internal stream=1, pts=8280, dts=8280, size=488 av_read_frame_internal stream=1, pts=11160, dts=11160, size=620 <!!!8280 av_read_frame_internal stream=1, pts=14040, dts=14040, size=488 av_read_packet stream=1, pts=9240, dts=9240, size=2016 305 av_read_frame_internal stream=1, pts=16920, dts=16920, size=620 av_read_frame_internal stream=1, pts=9240, dts=9240, size=488 av_read_frame_internal stream=1, pts=12120, dts=12120, size=620 <!!!9240 av_read_frame_internal stream=1, pts=15000, dts=15000, size=488 av_read_packet stream=1, pts=10200, dts=10200, size=2016 105 av_read_frame_internal stream=1, pts=17880, dts=17880, size=620 av_read_frame_internal stream=1, pts=10200, dts=10200, size=488 av_read_frame_internal stream=1, pts=13080, dts=13080, size=620 <!!!10200 av_read_packet stream=1, pts=10680, dts=10680, size=2016 393 av_read_frame_internal stream=1, pts=15960, dts=15960, size=488 av_read_frame_internal stream=1, pts=10680, dts=10680, size=620 <---10680 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
On Sun, Mar 30, 2008 at 03:16:03AM +0200, Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 08:16:24PM -0400, Justin Ruggles wrote:
Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote: >>> 3.change of ff_aac_ac3_parse to make it react correctly to result >>> returned in flag. > According to suggestions, I wrote new patch to AAC/AC3 parser. > > Current data about the stream are taken from frames FRAME_START and > FRAME_COMPLETE. It will be improved in the next patch. > -- > Bartlomiej Wolowiec > > Index: libavcodec/aac_ac3_parser.c > =================================================================== > --- libavcodec/aac_ac3_parser.c (wersja 12623) > +++ libavcodec/aac_ac3_parser.c (kopia robocza) > @@ -29,35 +29,50 @@ > const uint8_t *buf, int buf_size) > { > AACAC3ParseContext *s = s1->priv_data; > - AACAC3FrameFlag frame_flag; > const uint8_t *buf_ptr; > int len; > > *poutbuf = NULL; > *poutbuf_size = 0; > > + if(s->frame_ptr == NULL) { > + //after sending package of data in the end there was one new > header + memcpy(s->inbuf, s->frame_start, s->header_size); > + s->frame_start = s->inbuf; > + s->frame_ptr = s->frame_start + s->header_size; > + } Instead of this i think you could just return a smaller number. We do have code in the parser that does what is needed to move this to the begin (search for overread) in parser*. I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside
On sobota, 29 marca 2008, Michael Niedermayer wrote: the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean? I meant something that didnt exist. Sorry! Either way after looking at the parser code again and reading the spec again i
On Sat, Mar 29, 2008 at 10:04:30PM +0100, Bartlomiej Wolowiec wrote: think there are a few problems.
First could you explain me how these 7+ch EAC-3 streams are stored in MPEG? Second how are timestamps associated with them if they are stored in a single ES stream instead of 2. Can timestamps be associated with dependant "frames" or not? (If we do not know this its not possible to implement 7+ch EAC-3)
Either way, a parser as you implement it must at least return the correct index (which can be negative up to the header size) current suggested code returns something random. Also it requires at least an increase of AV_PARSER_PTS_NB. And this design requires that timestamps can not be associated with dependant "frames". If they can more changes or better a different design has to be choosen.
besides, do we have sample files for 7+ch EAC3 ? Especially ones muxed in MPEG?
We have 1 sample for 7.1 E-AC3. I can't find the original EVO that it came from, so I sent a message to the person who provided it originally. I'm sure madshi (eac3to author) still has it lying around somewhere as well... (are you subscribed here madshi?)
correction. We have 2 samples. I uploaded a different EVO sample to
http://samples.mplayerhq.hu/A-codecs/AC3/eac3/7_pt_1_sample.evo
Good news, Timestamps do only apply to the first of the 2 chunks, so we were on the right track. Returning the correct (possibly negative) index and changing AV_PARSER_PTS_NB should be correct ...
Also what was the exact reason that ff_combine_frame() wasnt used? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Let us carefully observe those good qualities wherein our enemies excel us and endeavor to excel them, by avoiding what is faulty, and imitating what is excellent in them. -- Plutarch
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 03:16:03AM +0200, Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 08:16:24PM -0400, Justin Ruggles wrote:
Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sat, Mar 29, 2008 at 10:04:30PM +0100, Bartlomiej Wolowiec wrote:
On sobota, 29 marca 2008, Michael Niedermayer wrote: > On Sat, Mar 29, 2008 at 12:19:43AM +0100, Bartlomiej Wolowiec wrote: >>>> 3.change of ff_aac_ac3_parse to make it react correctly to >>>> result returned in flag. >> >> According to suggestions, I wrote new patch to AAC/AC3 parser. >> >> Current data about the stream are taken from frames FRAME_START >> and FRAME_COMPLETE. It will be improved in the next patch. >> -- >> Bartlomiej Wolowiec >> >> Index: libavcodec/aac_ac3_parser.c >> ================================================================= >>== --- libavcodec/aac_ac3_parser.c (wersja 12623) >> +++ libavcodec/aac_ac3_parser.c (kopia robocza) >> @@ -29,35 +29,50 @@ >> const uint8_t *buf, int buf_size) >> { >> AACAC3ParseContext *s = s1->priv_data; >> - AACAC3FrameFlag frame_flag; >> const uint8_t *buf_ptr; >> int len; >> >> *poutbuf = NULL; >> *poutbuf_size = 0; >> >> + if(s->frame_ptr == NULL) { >> + //after sending package of data in the end there was one >> new header + memcpy(s->inbuf, s->frame_start, >> s->header_size); + s->frame_start = s->inbuf; >> + s->frame_ptr = s->frame_start + s->header_size; >> + } > > Instead of this i think you could just return a smaller number. > We do have code in the parser that does what is needed to move > this to the begin (search for overread) in parser*.
I've searched, but I haven't found anything appropriate. Majority of solutions is based on ff_combine_frame and it seeks 4 byte signature, but here, beside the signature other parameters shoulf be read (minimum 7 bytes forward should be in the buffer). Could you tell me what solution do you mean?
I meant something that didnt exist. Sorry! Either way after looking at the parser code again and reading the spec again i think there are a few problems.
First could you explain me how these 7+ch EAC-3 streams are stored in MPEG? Second how are timestamps associated with them if they are stored in a single ES stream instead of 2. Can timestamps be associated with dependant "frames" or not? (If we do not know this its not possible to implement 7+ch EAC-3)
Either way, a parser as you implement it must at least return the correct index (which can be negative up to the header size) current suggested code returns something random. Also it requires at least an increase of AV_PARSER_PTS_NB. And this design requires that timestamps can not be associated with dependant "frames". If they can more changes or better a different design has to be choosen.
besides, do we have sample files for 7+ch EAC3 ? Especially ones muxed in MPEG?
We have 1 sample for 7.1 E-AC3. I can't find the original EVO that it came from, so I sent a message to the person who provided it originally. I'm sure madshi (eac3to author) still has it lying around somewhere as well... (are you subscribed here madshi?)
correction. We have 2 samples. I uploaded a different EVO sample to
http://samples.mplayerhq.hu/A-codecs/AC3/eac3/7_pt_1_sample.evo
Good news, Timestamps do only apply to the first of the 2 chunks, so we were on the right track. Returning the correct (possibly negative) index and changing AV_PARSER_PTS_NB should be correct ...
Also what was the exact reason that ff_combine_frame() wasnt used?
[...]
Perfect, it's better that there are no timestamps in dependant frames... I don't know abilities of ff_combine_frame so good, so I don't know if there is a simple method to read the whole header of the next frame. -- Bartlomiej Wolowiec
On Sun, Mar 30, 2008 at 12:36:30PM +0100, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 03:16:03AM +0200, Michael Niedermayer wrote: [...] Also what was the exact reason that ff_combine_frame() wasnt used?
[...]
Perfect, it's better that there are no timestamps in dependant frames...
I don't know abilities of ff_combine_frame so good, so I don't know if there is a simple method to read the whole header of the next frame.
The header should just be there in the buffer with ff_combine_frame(). Anyway what i meant was that i _think_ the current aac/ac3 parser could be simplified by using ff_combine_frame(). The advanatges of ff_combine_frame() being that it takes care of the too much read header and doesnt by itself force a unneeded copy. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No great genius has ever existed without some touch of madness. -- Aristotle
On Sun, Mar 30, 2008 at 01:53:18PM +0200, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 12:36:30PM +0100, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 03:16:03AM +0200, Michael Niedermayer wrote: [...] Also what was the exact reason that ff_combine_frame() wasnt used?
[...]
Perfect, it's better that there are no timestamps in dependant frames...
I don't know abilities of ff_combine_frame so good, so I don't know if there is a simple method to read the whole header of the next frame.
The header should just be there in the buffer with ff_combine_frame(). Anyway what i meant was that i _think_ the current aac/ac3 parser could be simplified by using ff_combine_frame(). The advanatges of ff_combine_frame() being that it takes care of the too much read header and doesnt by itself force a unneeded copy.
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work. After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value. Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame() [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 01:53:18PM +0200, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 12:36:30PM +0100, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
On Sun, Mar 30, 2008 at 03:16:03AM +0200, Michael Niedermayer wrote:
[...]
Also what was the exact reason that ff_combine_frame() wasnt used?
[...]
Perfect, it's better that there are no timestamps in dependant frames...
I don't know abilities of ff_combine_frame so good, so I don't know if there is a simple method to read the whole header of the next frame.
The header should just be there in the buffer with ff_combine_frame(). Anyway what i meant was that i _think_ the current aac/ac3 parser could be simplified by using ff_combine_frame(). The advanatges of ff_combine_frame() being that it takes care of the too much read header and doesnt by itself force a unneeded copy.
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Ok, now I understand... I was just thinking how to make solution similar to other solutions.. I'll try in short time to write this parser with ff_combine_frame(), but now I'll try to finish my GSoC application, because the deadline is near... -- Bartlomiej Wolowiec
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution... Because of that I don't know what to do: -leave the code as it is, -use ff_combine_frame, -write new functions which will operate on the buffer (similar to ff_combine_frame, but adapted to the needs of aac_ac3_parser)? -- Bartlomiej Wolowiec
On Thu, Apr 03, 2008 at 11:53:22PM +0200, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution...
What i had in mind is below, this is blindly written and likely contains bugs. I did spot one each time i looked at it :) But it does not look fundamentally worse to me, it also has the advantage that its not full of unneeded memcpys and memmoves. while(s->remaining_size <= buf_size){ if(s->remaining_size && !s->need_next_header){ i= s->remaining_size; s->remaining_size= 0; goto output_frame; }else{ //we need a header first for(i=s->remaining_size; i<buf_size; i++){ ctx->state= (ctx->state<<8) + buf[i]; if((len=parse_header(ctx->state, &s->need_next_header, &s->new_frame_start))) break; } i-= header_size; if(len>0){ s->remaining_size= len + i; if(pc->index+i > 0 && s->new_frame_start){ s->remaining_size -= i; // remaining_size=len output_frame: ff_combine_frame(pc, i, &buf, &buf_size); *poutbuf = buf; *poutbuf_size = buf_size; return i; } }else{ break; } } } ff_combine_frame(pc, END_NOT_FOUND, &buf, &buf_size); s->remaining_size -= FFMIN(s->remaining_size, buf_size); *poutbuf = NULL; *poutbuf_size = 0; return buf_size;
Because of that I don't know what to do: -leave the code as it is, -use ff_combine_frame, -write new functions which will operate on the buffer (similar to ff_combine_frame, but adapted to the needs of aac_ac3_parser)?
Do what is simplest and fastest. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
On Fri, Apr 04, 2008 at 01:45:44AM +0200, Michael Niedermayer wrote:
On Thu, Apr 03, 2008 at 11:53:22PM +0200, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution...
What i had in mind is below, this is blindly written and likely contains bugs. I did spot one each time i looked at it :) But it does not look fundamentally worse to me, it also has the advantage that its not full of unneeded memcpys and memmoves.
while(s->remaining_size <= buf_size){ if(s->remaining_size && !s->need_next_header){ i= s->remaining_size; s->remaining_size= 0; goto output_frame; }else{ //we need a header first for(i=s->remaining_size; i<buf_size; i++){ ctx->state= (ctx->state<<8) + buf[i]; if((len=parse_header(ctx->state, &s->need_next_header, &s->new_frame_start)))
just to clarify before you ask ... state is a 64bit variable in your context unrelated to the 32bit state ff_combine_frame() plays with. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
On Fri, Apr 04, 2008 at 01:45:44AM +0200, Michael Niedermayer wrote:
On Thu, Apr 03, 2008 at 11:53:22PM +0200, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution...
What i had in mind is below, this is blindly written and likely contains bugs. I did spot one each time i looked at it :) But it does not look fundamentally worse to me, it also has the advantage that its not full of unneeded memcpys and memmoves.
while(s->remaining_size <= buf_size){ if(s->remaining_size && !s->need_next_header){ i= s->remaining_size; s->remaining_size= 0; goto output_frame; }else{ //we need a header first for(i=s->remaining_size; i<buf_size; i++){ ctx->state= (ctx->state<<8) + buf[i]; if((len=parse_header(ctx->state, &s->need_next_header, &s->new_frame_start)))
just to clarify before you ask ... state is a 64bit variable in your context unrelated to the 32bit state ff_combine_frame() plays with.
[...]
The code works almost perfectly, but I have two questions: -is there any effective method of getting bitstream from uint64_t (or it should be "unpacked" to 8-byte array)? -why the number of channels is set in parser? I have this question because the number of channels in eac3 depends on chanmap, to read which there it needs 92 bits of header. (or I should put there something like x = (x<<8) + (y>>56); y = (y<<8) + buf[i]; and keep 128 last bits?) -- Bartlomiej Wolowiec
On Fri, Apr 04, 2008 at 07:44:46PM +0200, Bartlomiej Wolowiec wrote:
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
On Fri, Apr 04, 2008 at 01:45:44AM +0200, Michael Niedermayer wrote:
On Thu, Apr 03, 2008 at 11:53:22PM +0200, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution...
What i had in mind is below, this is blindly written and likely contains bugs. I did spot one each time i looked at it :) But it does not look fundamentally worse to me, it also has the advantage that its not full of unneeded memcpys and memmoves.
while(s->remaining_size <= buf_size){ if(s->remaining_size && !s->need_next_header){ i= s->remaining_size; s->remaining_size= 0; goto output_frame; }else{ //we need a header first for(i=s->remaining_size; i<buf_size; i++){ ctx->state= (ctx->state<<8) + buf[i]; if((len=parse_header(ctx->state, &s->need_next_header, &s->new_frame_start)))
just to clarify before you ask ... state is a 64bit variable in your context unrelated to the 32bit state ff_combine_frame() plays with.
[...]
The code works almost perfectly, but I have two questions: -is there any effective method of getting bitstream from uint64_t (or it should be "unpacked" to 8-byte array)?
uint64_t tmp= be2me_64(state); init_get_bits(&tmp);
-why the number of channels is set in parser?
streamcopy mpeg-ps -> anything first doesnt have a header with a channel number second does.
I have this question because the number of channels in eac3 depends on chanmap, to read which there it needs 92 bits of header. (or I should put there something like x = (x<<8) + (y>>56); y = (y<<8) + buf[i]; and keep 128 last bits?)
well, thats a possibility. a buf[16]; memmove(buf) buf[15]= *b++; would be a possibility as well a third one (and i think its the best one) is just to read the header with the channel num before the returning it after ff_combine_frame(). It has to be a complete frame at that point, also please reuse the code from the decoder for this if possible! [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
On Fri, Apr 04, 2008 at 07:44:46PM +0200, Bartlomiej Wolowiec wrote:
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
On Fri, Apr 04, 2008 at 01:45:44AM +0200, Michael Niedermayer wrote:
On Thu, Apr 03, 2008 at 11:53:22PM +0200, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote:
Note, to add the header into the buffer calling ff_combine_frame() with END_NOT_FOUND and a smaller buffer size should work.
After its in the buffer you just run the header decoder thing over the header and when you know the final frame size you call ff_combine_frame with appropriate next value.
Anyway iam not insisting on you changing the current aac/ac3 parser to use ff_combine_frame() But what you will end up implementing will be quite similar in functionality to ff_combine_frame()
[...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution...
What i had in mind is below, this is blindly written and likely contains bugs. I did spot one each time i looked at it :) But it does not look fundamentally worse to me, it also has the advantage that its not full of unneeded memcpys and memmoves.
while(s->remaining_size <= buf_size){ if(s->remaining_size && !s->need_next_header){ i= s->remaining_size; s->remaining_size= 0; goto output_frame; }else{ //we need a header first for(i=s->remaining_size; i<buf_size; i++){ ctx->state= (ctx->state<<8) + buf[i]; if((len=parse_header(ctx->state, &s->need_next_header, &s->new_frame_start)))
just to clarify before you ask ... state is a 64bit variable in your context unrelated to the 32bit state ff_combine_frame() plays with.
[...]
The code works almost perfectly, but I have two questions: -is there any effective method of getting bitstream from uint64_t (or it should be "unpacked" to 8-byte array)?
uint64_t tmp= be2me_64(state); init_get_bits(&tmp);
-why the number of channels is set in parser?
streamcopy mpeg-ps -> anything first doesnt have a header with a channel number second does.
I have this question because the number of channels in eac3 depends on chanmap, to read which there it needs 92 bits of header. (or I should put there something like x = (x<<8) + (y>>56); y = (y<<8) + buf[i]; and keep 128 last bits?)
well, thats a possibility. a buf[16]; memmove(buf) buf[15]= *b++; would be a possibility as well
a third one (and i think its the best one) is just to read the header with the channel num before the returning it after ff_combine_frame(). It has to be a complete frame at that point, also please reuse the code from the decoder for this if possible!
[...]
ok, I enclose patch based on your code. -- Bartlomiej Wolowiec
On Fri, Apr 04, 2008 at 11:58:09PM +0200, Bartlomiej Wolowiec wrote:
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
On Fri, Apr 04, 2008 at 07:44:46PM +0200, Bartlomiej Wolowiec wrote:
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
On Fri, Apr 04, 2008 at 01:45:44AM +0200, Michael Niedermayer wrote:
On Thu, Apr 03, 2008 at 11:53:22PM +0200, Bartlomiej Wolowiec wrote:
On niedziela, 30 marca 2008, Michael Niedermayer wrote: > Note, to add the header into the buffer calling > ff_combine_frame() with END_NOT_FOUND and a smaller buffer size > should work. > > After its in the buffer you just run the header decoder thing > over the header and when you know the final frame size you call > ff_combine_frame with appropriate next value. > > Anyway iam not insisting on you changing the current aac/ac3 > parser to use ff_combine_frame() > But what you will end up implementing will be quite similar in > functionality to ff_combine_frame() > > [...]
Hmm... I tried to implement it using ff_combine_frame, but the code which I got is in my opinion significantly worse. Main problems which I had: -ff_combine_frame is rather adapted to using on one frame, when in buffer is one frame and it currently reads the next one it isn't convenient (the whole buffer size is not just a new frame's size, so that we should remember size of earlier frames). -in the genuine code there is memmove(s->frame_start, s->frame_start + 1, s->header_size - 1); s->frame_ptr--; here we can operate on pc->buffer and pc->index (remembering also where the new frame begins). However, surely it isn't the simpliest solution...
What i had in mind is below, this is blindly written and likely contains bugs. I did spot one each time i looked at it :) But it does not look fundamentally worse to me, it also has the advantage that its not full of unneeded memcpys and memmoves.
while(s->remaining_size <= buf_size){ if(s->remaining_size && !s->need_next_header){ i= s->remaining_size; s->remaining_size= 0; goto output_frame; }else{ //we need a header first for(i=s->remaining_size; i<buf_size; i++){ ctx->state= (ctx->state<<8) + buf[i]; if((len=parse_header(ctx->state, &s->need_next_header, &s->new_frame_start)))
just to clarify before you ask ... state is a 64bit variable in your context unrelated to the 32bit state ff_combine_frame() plays with.
[...]
The code works almost perfectly, but I have two questions: -is there any effective method of getting bitstream from uint64_t (or it should be "unpacked" to 8-byte array)?
uint64_t tmp= be2me_64(state); init_get_bits(&tmp);
-why the number of channels is set in parser?
streamcopy mpeg-ps -> anything first doesnt have a header with a channel number second does.
I have this question because the number of channels in eac3 depends on chanmap, to read which there it needs 92 bits of header. (or I should put there something like x = (x<<8) + (y>>56); y = (y<<8) + buf[i]; and keep 128 last bits?)
well, thats a possibility. a buf[16]; memmove(buf) buf[15]= *b++; would be a possibility as well
a third one (and i think its the best one) is just to read the header with the channel num before the returning it after ff_combine_frame(). It has to be a complete frame at that point, also please reuse the code from the decoder for this if possible!
[...]
ok, I enclose patch based on your code.
Looks ok iff it works. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle
On sobota, 5 kwietnia 2008, Michael Niedermayer wrote:
ok, I enclose patch based on your code.
Looks ok iff it works.
[...]
I've tested thoroughly and I've found a little bug: len may be not initialized when s->remaining_size == buf_size. -- Bartlomiej Wolowiec
On Sat, Apr 05, 2008 at 05:14:14PM +0200, Bartlomiej Wolowiec wrote:
On sobota, 5 kwietnia 2008, Michael Niedermayer wrote:
ok, I enclose patch based on your code.
Looks ok iff it works.
[...]
I've tested thoroughly and I've found a little bug: len may be not initialized when s->remaining_size == buf_size.
looks ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus
On sobota, 5 kwietnia 2008, Michael Niedermayer wrote:
looks ok
[...]
patch applied -- Bartlomiej Wolowiec
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
a third one (and i think its the best one) is just to read the header with the channel num before the returning it after ff_combine_frame(). It has to be a complete frame at that point, also please reuse the code from the decoder for this if possible!
Ok, according to your suggestion, I will read the number of channels when there is complete frame. Is it a good idea to make a small extension of ac3_parser and add to it an argument which shows if channel_map should be read and then add ac3_parser call at the end of aac_ac3_parser...? -- Bartlomiej Wolowiec
On Wed, Apr 09, 2008 at 10:16:50AM +0200, Bartlomiej Wolowiec wrote:
On piątek, 4 kwietnia 2008, Michael Niedermayer wrote:
a third one (and i think its the best one) is just to read the header with the channel num before the returning it after ff_combine_frame(). It has to be a complete frame at that point, also please reuse the code from the decoder for this if possible!
Ok, according to your suggestion, I will read the number of channels when there is complete frame. Is it a good idea to make a small extension of ac3_parser and add to it an argument which shows if channel_map should be read and then add ac3_parser call at the end of aac_ac3_parser...?
id just add a parse_header() call to aac_ac3_parser() which is always executed when a frame is finished. So that parse_header() would be like sync(). The code below "/* allow downmixing to stereo (or mono for AC3) */"could also be moved into this parse_header() [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
Bartlomiej Wolowiec wrote:
2008/3/26, Michael Niedermayer <michaelni@gmx.at>:
I do not like generic code full of If(codecA) else
The sync function should return if it has a complete frame or if (maybe) not. That is
flags= FRAME_START | FRAME_END;
You do this in stream_type but this name is totally wrong this is not a stream_type. It is not even constant for a stream. NEVER even think of using a name from the a52* spec, the people who wrote it are idiots. Its a flag indicating the start/end of a frame. And IMHO it should be returned more directly than as a context variable but thats rather minor the name is a major issue. It totally confused me until looked it up in the spec.
Code should be understandable without looking things up in the specs.
Also the "stream_type" has the wrong type (should be enum) and the types used in AAC-AC3 code must be generic types not *AC3 specific types in headers which arent #included.
Ok, what do you think about such a plan:
Also keep in mind (in case you are not already aware) that a normal AC3 frame can be followed by a dependent E-AC3 frame, usually containing the 7th and 8th channels of a 7.1 stream. This is supposedly the only way to carry a Dolby Digital 7.1 stream on Blu-ray. I have not found any samples though. -Justin
Bartlomiej Wolowiec wrote:
Currently, I'm trying to make changes which allow opening files with more channels than 5.1. Now the decoder is written in a way, which allows ac3_decode_frame to decode one frame, but additional channels in E-AC3 are sent in separate frames. Due to this, I have a question, how to solve it: - ac3_decode_frame function should decode many frames to get complete information about outstream, or - ac3_decode_frame function should return outstream data when it is called for the first frame from the next data package?
I definitely agree with Michael that the parser should send multiple AC3 frames as a single unit to the decoder. So the decoder will have to essentially process 2 (or more) frames at the same time. My suggestion is to break up the decode context since you will have to keep state information for both frames. Because each block is processed sequentially, the decoding process would need to be split up as well so that you can decode the transform coefficients from all AC3 frames in the set for a single block before downmixing. I understand that this work is a continuation of last year, but because the decoder is getting to a point where we want to add it to FFmpeg SVN soon, please post a patch of your solution to this before committing it. Thanks, Justin
participants (5)
-
Bartlomiej Wolowiec -
bwolowiec -
Diego Biurrun -
Justin Ruggles -
Michael Niedermayer