[2/2] movdec: Set frame_size for AMR

Message ID 1320413121-75123-2-git-send-email-martin@martin.st
State Deferred
Headers show

Commit Message

Martin Storsjö Nov. 4, 2011, 1:25 p.m.
From: Carl Eugen Hoyos <cehoyos@ag.or.at>

The amr demuxer also sets the frame_size field.
---
 libavformat/mov.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

Luca Barbato Nov. 4, 2011, 2:33 p.m. | #1
On 11/4/11 6:25 AM, Martin Storsjö wrote:
> From: Carl Eugen Hoyos<cehoyos@ag.or.at>
>
> The amr demuxer also sets the frame_size field.
> ---

I'd use a switch.

lu
Martin Storsjö Nov. 4, 2011, 2:40 p.m. | #2
On Fri, 4 Nov 2011, Luca Barbato wrote:

> On 11/4/11 6:25 AM, Martin Storsjö wrote:
>> From: Carl Eugen Hoyos<cehoyos@ag.or.at>
>> 
>> The amr demuxer also sets the frame_size field.
>> ---
>
> I'd use a switch.

This actually alread is within a larger switch, with two statements being 
common to the both AMR forms, while the sample rate and frame size differ. 
Having two nested switches for the same thing looks just weird IMO, and 
duplicating the two common statements feels needless.

// Martin
Justin Ruggles Nov. 4, 2011, 2:45 p.m. | #3
On 11/04/2011 09:25 AM, Martin Storsjö wrote:

> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> 
> The amr demuxer also sets the frame_size field.


it doesn't need to though. it already sets pkt->duration.

> ---
>  libavformat/mov.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 2036f51..131e2b4 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1295,10 +1295,13 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext *pb, int entries)
>          st->codec->frame_size= sc->samples_per_frame;
>          st->codec->channels= 1; /* really needed */
>          /* force sample rate for amr, stsd in 3gp does not store sample rate */
> -        if (st->codec->codec_id == CODEC_ID_AMR_NB)
> +        if (st->codec->codec_id == CODEC_ID_AMR_NB) {
>              st->codec->sample_rate = 8000;
> -        else if (st->codec->codec_id == CODEC_ID_AMR_WB)
> +            st->codec->frame_size  = 160;
> +        } else if (st->codec->codec_id == CODEC_ID_AMR_WB) {
>              st->codec->sample_rate = 16000;
> +            st->codec->frame_size  = 320;
> +        }
>          break;
>      case CODEC_ID_MP2:
>      case CODEC_ID_MP3:


On the input side, the only thing frame_size is used for is guessing
packet duration.  The mov/mp4 demuxer already has other ways of
determining correct packet duration.  I don't think this change actually
does anything.

-Justin
Martin Storsjö Nov. 4, 2011, 2:52 p.m. | #4
On Fri, 4 Nov 2011, Justin Ruggles wrote:

> On 11/04/2011 09:25 AM, Martin Storsjö wrote:
>
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>
>> The amr demuxer also sets the frame_size field.
>
>
> it doesn't need to though. it already sets pkt->duration.
>
>> ---
>>  libavformat/mov.c |    7 +++++--
>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 2036f51..131e2b4 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -1295,10 +1295,13 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext *pb, int entries)
>>          st->codec->frame_size= sc->samples_per_frame;
>>          st->codec->channels= 1; /* really needed */
>>          /* force sample rate for amr, stsd in 3gp does not store sample rate */
>> -        if (st->codec->codec_id == CODEC_ID_AMR_NB)
>> +        if (st->codec->codec_id == CODEC_ID_AMR_NB) {
>>              st->codec->sample_rate = 8000;
>> -        else if (st->codec->codec_id == CODEC_ID_AMR_WB)
>> +            st->codec->frame_size  = 160;
>> +        } else if (st->codec->codec_id == CODEC_ID_AMR_WB) {
>>              st->codec->sample_rate = 16000;
>> +            st->codec->frame_size  = 320;
>> +        }
>>          break;
>>      case CODEC_ID_MP2:
>>      case CODEC_ID_MP3:
>
>
> On the input side, the only thing frame_size is used for is guessing
> packet duration.  The mov/mp4 demuxer already has other ways of
> determining correct packet duration.  I don't think this change actually
> does anything.

That might very well be the case, patch dropped.

// Martin

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2036f51..131e2b4 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1295,10 +1295,13 @@  int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext *pb, int entries)
         st->codec->frame_size= sc->samples_per_frame;
         st->codec->channels= 1; /* really needed */
         /* force sample rate for amr, stsd in 3gp does not store sample rate */
-        if (st->codec->codec_id == CODEC_ID_AMR_NB)
+        if (st->codec->codec_id == CODEC_ID_AMR_NB) {
             st->codec->sample_rate = 8000;
-        else if (st->codec->codec_id == CODEC_ID_AMR_WB)
+            st->codec->frame_size  = 160;
+        } else if (st->codec->codec_id == CODEC_ID_AMR_WB) {
             st->codec->sample_rate = 16000;
+            st->codec->frame_size  = 320;
+        }
         break;
     case CODEC_ID_MP2:
     case CODEC_ID_MP3: