[FFmpeg-devel] [PATCH v5 4/4] lavf/utils: Fix endianness in ff_get_packet_palette()

[Mats Peterson]

On 03/03/2016 03:05 AM, Michael Niedermayer wrote:
> On Thu, Mar 03, 2016 at 12:36:21AM +0100, Mats Peterson wrote:
>>
>> --
>> Mats Peterson
>> http://matsp888.no-ip.org/~mats/
>
>>   internal.h |    7 +++++--
>>   utils.c    |   23 ++++++++++++++++-------
>>   2 files changed, 21 insertions(+), 9 deletions(-)
>> 3f86328b63621ddba1ee0730ce639010fc38aec3  0004-lavf-utils-Fix-endianness-in-ff_get_palette_packet.patch
>>  From 75f98197bda12b486971d1d6cc139cb5d22b30ba Mon Sep 17 00:00:00 2001
>> From: Mats Peterson <matsp888 at yahoo.com>
>> Date: Thu, 3 Mar 2016 00:29:32 +0100
>> Subject: [PATCH v5 4/4] lavf/utils: Fix endianness in ff_get_packet_palette()
>>
>> ---
>>   libavformat/internal.h |    7 +++++--
>>   libavformat/utils.c    |   23 ++++++++++++++++-------
>>   2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index 8e06655..e58d078 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -580,9 +580,12 @@ int ff_reshuffle_raw_rgb(AVFormatContext *s, AVPacket **ppkt, AVCodecContext *en
>>    *
>>    * Use 0 for the ret parameter to check for side data only.
>>    *
>> - * @param pkt pointer to the packet before calling ff_reshuffle_raw_rgb()
>> + * @param pkt pointer to packet before calling ff_reshuffle_raw_rgb()
>>    * @param ret return value from ff_reshuffle_raw_rgb(), or 0
>> + * @param palette pointer to palette buffer
>> + * @return negative error code or
>> +           1 if the packet contains a palette, else 0
>>    */
>> -int ff_get_packet_palette(AVFormatContext *s, AVPacket *pkt, int ret, const uint8_t **palette);
>> +int ff_get_packet_palette(AVFormatContext *s, AVPacket *pkt, int ret, uint32_t *palette);
>>
>>   #endif /* AVFORMAT_INTERNAL_H */
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index 771e878..9931e61 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -4760,18 +4760,27 @@ int ff_parse_creation_time_metadata(AVFormatContext *s, int64_t *timestamp, int
>>       return 0;
>>   }
>>
>> -int ff_get_packet_palette(AVFormatContext *s, AVPacket *pkt, int ret, const uint8_t **palette)
>> +int ff_get_packet_palette(AVFormatContext *s, AVPacket *pkt, int ret, uint32_t *palette)
>>   {
>> +    uint8_t *side_data;
>>       int size;
>>
>> -    *palette = av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE, &size);
>> -    if (*palette && size != AVPALETTE_SIZE) {
>> -        av_log(s, AV_LOG_ERROR, "Invalid palette side data\n");
>> -        return AVERROR_INVALIDDATA;
>> +    side_data = av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE, &size);
>> +    if (side_data) {
>> +        if (size != AVPALETTE_SIZE) {
>> +            av_log(s, AV_LOG_ERROR, "Invalid palette side data\n");
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +        memcpy(palette, side_data, AVPALETTE_SIZE);
>> +        return 1;
>>       }
>>
>> -    if (!*palette && ret == CONTAINS_PAL)
>> -        *palette = pkt->data + pkt->size - AVPALETTE_SIZE;
>> +    if (ret == CONTAINS_PAL) {
>> +        int i;
>> +        for (i = 0; i < AVPALETTE_COUNT; i++)
>> +            palette[i] = AV_RL32(pkt->data + pkt->size - AVPALETTE_SIZE + i*4);
>> +        return 1;
>> +    }
>>
>>       return 0;
>>   }
>
> with this API each muxer must convert the palette from native endian
> to whatever the format requires

I don't understand what you mean here really.

> feels a bit odd to extact the palette from AVPacket.data where its
> stored as the container needs (for some containers) and then
> convert that to RGBA in native int32 which the muxer then needs to
> convert back for storage
>

I don't think so. Since any palette side data, as you said yourself, 
uses native endian, I have made this function return the palette in 
native order "to the outside world". It's more logical than having to 
deal with the (for some weird reason) little-endian order of the 
AVPacket.data palette in certain places.

The muxer will have to convert it to what the storage needs for the 
specific file format, whether it's big- or little endian. That's 
irrelevant of the native endian of the palette in FFmpeg. These types of 
conversions (using av_wl32() or av_wb32() or similar) are done all over 
the place already, so it's nothing new.

I hope I have understood you correctly.

Mats