Re: [libav-devel] [PATCH 1/5] mpegaudio_parser: reset state to prevent it to be random

2013-02-17 Thread Reinhard Tartler 
On Fri, Oct 19, 2012 at 8:04 PM, Luca Barbato  wrote:
> On 10/19/2012 06:42 PM, Reinhard Tartler wrote:
>> On Wed, Oct 17, 2012 at 11:46 PM, Reinhard Tartler  
>> wrote:
>>> On Wed, Oct 17, 2012 at 11:32 PM, Måns Rullgård  wrote:
 Reinhard Tartler  writes:

> From: Michael Niedermayer 
>
> Fixes Ticket1718
>
> Signed-off-by: Michael Niedermayer 
> (cherry picked from commit 93b240f4a59348c07d3d7e4862227f6949c51e14)
>
> Signed-off-by: Reinhard Tartler 
> ---
>  libavcodec/mpegaudio_parser.c |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
> index c904873..4166f3f 100644
> --- a/libavcodec/mpegaudio_parser.c
> +++ b/libavcodec/mpegaudio_parser.c
> @@ -54,6 +54,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>  int inc= FFMIN(buf_size - i, s->frame_size);
>  i += inc;
>  s->frame_size -= inc;
> +state = 0;
>
>  if(!s->frame_size){
>  next= i;
>
> Big picture:
>
> uint32_t state= pc->state;
> int i;
> int next= END_NOT_FOUND;
>
> for(i=0; i if(s->frame_size){
> int inc= FFMIN(buf_size - i, s->frame_size);
> i += inc;
> s->frame_size -= inc;
>
> if(!s->frame_size){
> next= i;
> break;
>
> break;
> }
> }else{
> while(i int ret, sr, channels, bit_rate, frame_size;
>
> state= (state<<8) + buf[i++];
>
> ...
>
> in libavcodec/parser.c
>
> s = av_mallocz(sizeof(AVCodecParserContext));
>
> So the state starts 0, in the file then it gets
>
> the value set and so on. Nothing random here at all.
>
> "state" is fed to the header parser, so basically the patch proposes to
> reset it when you have a frame so you start afresh, the patch doesn't
> seem wrong, not sure if setting it on the !s->frame_size branch is more
> correct since I hadn't read the whole code, surely not using the
> previous buffer tail sounds correct.
>
> Here if you want to have a better look on more samples.
>
> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
> index c904873..e9eee1f 100644
> --- a/libavcodec/mpegaudio_parser.c
> +++ b/libavcodec/mpegaudio_parser.c
> @@ -49,13 +49,19 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>  int i;
>  int next= END_NOT_FOUND;
>
> +av_log(NULL, AV_LOG_INFO, "state start %d\n", state);
> +
>  for(i=0; i  if(s->frame_size){
>  int inc= FFMIN(buf_size - i, s->frame_size);
>  i += inc;
>  s->frame_size -= inc;
> +state = 0;
> +
> +av_log(NULL, AV_LOG_INFO, "state %d\n", state);
>
>  if(!s->frame_size){
> +state = 0;
>  next= i;
>  break;
>  }
> @@ -87,7 +93,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>  }
>  }
>  }
> -
> +av_log(NULL, AV_LOG_INFO, "state end %d\n", state);
>  pc->state= state;
>  if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
>  *poutbuf = NULL;

I believe the av_log could also b made AV_LOG_DEBUG. So, how about
taking Luca's patch with a commit message along the lines:

mpegaudio_parser: reset parsing context state on new frames


?
-- 
regards,
Reinhard
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/5] mpegaudio_parser: reset state to prevent it to be random

2012-10-19 Thread Luca Barbato 
On 10/19/2012 06:42 PM, Reinhard Tartler wrote:
> On Wed, Oct 17, 2012 at 11:46 PM, Reinhard Tartler  wrote:
>> On Wed, Oct 17, 2012 at 11:32 PM, Måns Rullgård  wrote:
>>> Reinhard Tartler  writes:
>>>
 From: Michael Niedermayer 

 Fixes Ticket1718

 Signed-off-by: Michael Niedermayer 
 (cherry picked from commit 93b240f4a59348c07d3d7e4862227f6949c51e14)

 Signed-off-by: Reinhard Tartler 
 ---
  libavcodec/mpegaudio_parser.c |1 +
  1 file changed, 1 insertion(+)

 diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
 index c904873..4166f3f 100644
 --- a/libavcodec/mpegaudio_parser.c
 +++ b/libavcodec/mpegaudio_parser.c
 @@ -54,6 +54,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
  int inc= FFMIN(buf_size - i, s->frame_size);
  i += inc;
  s->frame_size -= inc;
 +state = 0;

  if(!s->frame_size){
  next= i;

Big picture:

uint32_t state= pc->state;
int i;
int next= END_NOT_FOUND;

for(i=0; iframe_size){
int inc= FFMIN(buf_size - i, s->frame_size);
i += inc;
s->frame_size -= inc;

if(!s->frame_size){
next= i;
break;

break;
}
}else{
while(iframe_size branch is more
correct since I hadn't read the whole code, surely not using the
previous buffer tail sounds correct.

Here if you want to have a better look on more samples.

diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
index c904873..e9eee1f 100644
--- a/libavcodec/mpegaudio_parser.c
+++ b/libavcodec/mpegaudio_parser.c
@@ -49,13 +49,19 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
 int i;
 int next= END_NOT_FOUND;

+av_log(NULL, AV_LOG_INFO, "state start %d\n", state);
+
 for(i=0; iframe_size){
 int inc= FFMIN(buf_size - i, s->frame_size);
 i += inc;
 s->frame_size -= inc;
+state = 0;
+
+av_log(NULL, AV_LOG_INFO, "state %d\n", state);

 if(!s->frame_size){
+state = 0;
 next= i;
 break;
 }
@@ -87,7 +93,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
 }
 }
 }
-
+av_log(NULL, AV_LOG_INFO, "state end %d\n", state);
 pc->state= state;
 if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
 *poutbuf = NULL;


lu





___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/5] mpegaudio_parser: reset state to prevent it to be random

2012-10-19 Thread Reinhard Tartler 
On Wed, Oct 17, 2012 at 11:46 PM, Reinhard Tartler  wrote:
> On Wed, Oct 17, 2012 at 11:32 PM, Måns Rullgård  wrote:
>> Reinhard Tartler  writes:
>>
>>> From: Michael Niedermayer 
>>>
>>> Fixes Ticket1718
>>>
>>> Signed-off-by: Michael Niedermayer 
>>> (cherry picked from commit 93b240f4a59348c07d3d7e4862227f6949c51e14)
>>>
>>> Signed-off-by: Reinhard Tartler 
>>> ---
>>>  libavcodec/mpegaudio_parser.c |1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
>>> index c904873..4166f3f 100644
>>> --- a/libavcodec/mpegaudio_parser.c
>>> +++ b/libavcodec/mpegaudio_parser.c
>>> @@ -54,6 +54,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>>>  int inc= FFMIN(buf_size - i, s->frame_size);
>>>  i += inc;
>>>  s->frame_size -= inc;
>>> +state = 0;
>>>
>>>  if(!s->frame_size){
>>>  next= i;
>>> --
>>
>> What does this fix?
>
> Quoting from: https://ffmpeg.org/trac/ffmpeg/ticket/1718
>
> MP3 parser get the frame_size wrong when playback,
> the second and third frame_size are wrong.
>
> On the following samples file:
> https://ffmpeg.org/trac/ffmpeg/raw-attachment/ticket/1718/M4A_MP1_288Kbps_CBR_44.1KHz_2ch.m4a,
> avprobe changes its output like this:
>
>
> --- /tmp/buggy  2012-10-17 23:44:40.595713001 +0200
> +++ /tmp/fixed  2012-10-17 23:44:46.275712847 +0200
> @@ -22,7 +22,7 @@
>  dts_time=0.008685
>  duration=384
>  duration_time=0.008707
> -size=333.00
> +size=488.00
>  pos=167959
>  flags=K
>
> @@ -35,8 +35,8 @@
>  dts_time=0.017392
>  duration=384
>  duration_time=0.008707
> -size=643.00
> -pos=-1
> +size=488.00
> +pos=168447
>  flags=K
>
>  [packets.packet.3]

Opinions on this?

-- 
regards,
Reinhard
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/5] mpegaudio_parser: reset state to prevent it to be random

2012-10-17 Thread Reinhard Tartler 
On Wed, Oct 17, 2012 at 11:32 PM, Måns Rullgård  wrote:
> Reinhard Tartler  writes:
>
>> From: Michael Niedermayer 
>>
>> Fixes Ticket1718
>>
>> Signed-off-by: Michael Niedermayer 
>> (cherry picked from commit 93b240f4a59348c07d3d7e4862227f6949c51e14)
>>
>> Signed-off-by: Reinhard Tartler 
>> ---
>>  libavcodec/mpegaudio_parser.c |1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
>> index c904873..4166f3f 100644
>> --- a/libavcodec/mpegaudio_parser.c
>> +++ b/libavcodec/mpegaudio_parser.c
>> @@ -54,6 +54,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>>  int inc= FFMIN(buf_size - i, s->frame_size);
>>  i += inc;
>>  s->frame_size -= inc;
>> +state = 0;
>>
>>  if(!s->frame_size){
>>  next= i;
>> --
>
> What does this fix?

Quoting from: https://ffmpeg.org/trac/ffmpeg/ticket/1718

MP3 parser get the frame_size wrong when playback,
the second and third frame_size are wrong.

On the following samples file:
https://ffmpeg.org/trac/ffmpeg/raw-attachment/ticket/1718/M4A_MP1_288Kbps_CBR_44.1KHz_2ch.m4a,
avprobe changes its output like this:


--- /tmp/buggy  2012-10-17 23:44:40.595713001 +0200
+++ /tmp/fixed  2012-10-17 23:44:46.275712847 +0200
@@ -22,7 +22,7 @@
 dts_time=0.008685
 duration=384
 duration_time=0.008707
-size=333.00
+size=488.00
 pos=167959
 flags=K

@@ -35,8 +35,8 @@
 dts_time=0.017392
 duration=384
 duration_time=0.008707
-size=643.00
-pos=-1
+size=488.00
+pos=168447
 flags=K

 [packets.packet.3]


-- 
regards,
Reinhard
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/5] mpegaudio_parser: reset state to prevent it to be random

2012-10-17 Thread Måns Rullgård 
Reinhard Tartler  writes:

> From: Michael Niedermayer 
>
> Fixes Ticket1718
>
> Signed-off-by: Michael Niedermayer 
> (cherry picked from commit 93b240f4a59348c07d3d7e4862227f6949c51e14)
>
> Signed-off-by: Reinhard Tartler 
> ---
>  libavcodec/mpegaudio_parser.c |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
> index c904873..4166f3f 100644
> --- a/libavcodec/mpegaudio_parser.c
> +++ b/libavcodec/mpegaudio_parser.c
> @@ -54,6 +54,7 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>  int inc= FFMIN(buf_size - i, s->frame_size);
>  i += inc;
>  s->frame_size -= inc;
> +state = 0;
>
>  if(!s->frame_size){
>  next= i;
> -- 

What does this fix?

-- 
Måns Rullgård
m...@mansr.com
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel