[FFmpeg-devel] [PATCH] avformat/mov: ensure pasp box derived SAR is used if present

[James Almer]

On 7/10/2024 1:02 PM, Leo Izen wrote:
> On 7/9/24 1:21 PM, James Almer wrote:
>> It's meant to override any codec specific (but container level) 
>> information,
>> but its position is not guaranteed, so apply the values after the 
>> entire trak
>> structure has been parsed.
>> Also, replace the ugly roundabout int -> double -> int method to set 
>> SAR from
>> existing dimensions while at it.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavformat/isom.h |  2 ++
>>   libavformat/mov.c  | 16 ++++++++++++----
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index a0498f45e5..5b6125a908 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -215,6 +215,8 @@ typedef struct MOVStreamContext {
>>       int timecode_track;
>>       int width;            ///< tkhd width
>>       int height;           ///< tkhd height
>> +    int h_spacing;        ///< pasp hSpacing
>> +    int v_spacing;        ///< pasp vSpacing
>>       int dts_shift;        ///< dts shift when ctts is negative
>>       uint32_t palette[256];
>>       int has_palette;
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index ced4b2e6b3..ce95842ce5 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -1287,14 +1287,18 @@ static int mov_read_pasp(MOVContext *c, 
>> AVIOContext *pb, MOVAtom atom)
>>       const int num = avio_rb32(pb);
>>       const int den = avio_rb32(pb);
>>       AVStream *st;
>> +    MOVStreamContext *sc;
>>       if (c->fc->nb_streams < 1)
>>           return 0;
>>       st = c->fc->streams[c->fc->nb_streams-1];
>> +    sc = st->priv_data;
>> +
>> +    av_log(c->fc, AV_LOG_TRACE, "pasp: hSpacing %d, vSpacing %d\n", 
>> num, den);
>>       if (den != 0) {
>> -        av_reduce(&st->sample_aspect_ratio.num, 
>> &st->sample_aspect_ratio.den,
>> -                  num, den, 32767);
>> +        sc->h_spacing = num;
>> +        sc->v_spacing = den;
>>       }
> 
> If den == 0, nothing is assigned at all, but if den != 0 and if num == 
> 0, we assign 0 to h_spacing, which will end up doing nothing because we 
> check h_spacing && v_spacing later on in the other line you add.
> 
> Is this intentional? Is this desired behavior? etc.

A 0/x SAR is just 0, or unset, regardless of the value for den, and we 
check for sar.num immediately after the "sc->h_spacing && sc->v_spacing" 
one and set it to something if it's 0, so it will make no difference.