[FFmpeg-devel] [PATCH] Correct detection of EA_ADPCM_Rx in EA chunked files with PTxx header

[Aurelien Jacobs]

Jonathan Wilson wrote:

> This patch adds code to correctly detect EA_ADPCM_R1/R2 in EA chunked files 
> that contain a PTxx header chunk.
> It's based on the patch from Peter Ross.

It would have been faster for me to adapt your patch and commit, but I will
try to be didactic and comment it so that you have a chance to improve your
next patches.

> Index: libavformat/electronicarts.c
> ===================================================================
> --- libavformat/electronicarts.c	(revision 10885)
> +++ libavformat/electronicarts.c	(working copy)
> @@ -161,16 +167,39 @@
>          case  1: ea->audio_codec = CODEC_ID_ADPCM_EA_R1; break;
>          case  2: ea->audio_codec = CODEC_ID_ADPCM_EA_R2; break;
>          case  3: ea->audio_codec = CODEC_ID_ADPCM_EA_R3; break;
> +        case -1: break;
>          default:
>              av_log(s, AV_LOG_ERROR, "unsupported stream type;
> revision=%i\n", revision); return 0;
>          }
> +        switch(revision2) {
> +        case 0xa:

I see no points in using hexa here, when all other cases use decimal.

> +            if (chunk_type==PT50_TAG) {

Other places in this file have spaces around ==.

> +              ea->audio_codec = CODEC_ID_ADPCM_EA_R2;

Wrong indentation.

> +            }else if (chunk_type==PT70_TAG) {

space before else and around ==.

> +              ea->audio_codec = CODEC_ID_ADPCM_EA_R1;

indentation

> [...]
> 
> +    if (!ea->audio_codec) {
> +        ea->audio_codec = ea->bytes==1 ? CODEC_ID_PCM_S8 : CODEC_ID_PCM_S16LE;
> +    }

ea->bytes is forced to 2 in this function so no needs to check it.
Moreover, is there any sample for which setting ea->audio_codec to
CODEC_ID_PCM_S16LE here is useful ?
Anyway, this chunk is unrelated to the current patch so it needs
to be split.

> +    if (ea->num_channels == -1) {
> +        av_log (s, AV_LOG_ERROR, "unsupported stream type;
> num_channels undefined\n");
> +        return 0;
> +    }

Unrelated to this patch, and this code can't be reached anyway !

>      if (ea->sample_rate == -1)
>          ea->sample_rate = revision==3 ? 48000 : 22050;
>  
> @@ -273,11 +302,11 @@
>                  blockid = get_le32(pb);
>                  if (blockid == GSTR_TAG) {
>                      url_fskip(pb, 4);
> -                } else if (blockid != PT00_TAG) {
> +                } else if ((blockid != PT00_TAG) && (blockid != PT50_TAG) && (blockid != PT70_TAG)) {

overly long line and useless parenthesis.

>                      av_log (s, AV_LOG_ERROR, "unknown SCHl headerid\n");
>                      return 0;
>                  }
> -                err = process_audio_header_elements(s);
> +                err = process_audio_header_elements(s,blockid);

space after ,

> @@ -401,6 +430,12 @@
>                      ea->audio_frame_counter += ((chunk_size - 12) *
> 2) / ea->num_channels;
>                          break;
> +                    case CODEC_ID_ADPCM_EA_R1:
> +                    case CODEC_ID_ADPCM_EA_R2:
> +                    case CODEC_ID_ADPCM_EA_R3:
> +                        ea->audio_frame_counter += ((chunk_size - (12 + 4*ea->num_channels)) * 2) / 
> +                        ea->num_channels;
> +                        break;

Unrelated to this patch, and ugly split for this overly long line.

Aurel