[FFmpeg-devel] [PATCH]Simplify targa decoding

[Paul B Mahol]

On 1/17/12, Reimar Doeffinger <Reimar.Doeffinger at gmx.de> wrote:
> On Tue, Jan 17, 2012 at 11:51:42PM +0100, Jean First wrote:
>> On Tue Jan 17 23:49:18 2012, Carl Eugen Hoyos wrote:
>> >On Tuesday 17 January 2012 11:34:23 pm Reimar Doeffinger wrote:
>> >>On Tue, Jan 17, 2012 at 11:22:59PM +0100, Carl Eugen Hoyos wrote:
>> >>>diff --git a/libavcodec/targa.c b/libavcodec/targa.c
>> >>>index 57a4fee..0210133 100644
>> >>>--- a/libavcodec/targa.c
>> >>>+++ b/libavcodec/targa.c
>> >>>@@ -70,7 +70,7 @@ static int targa_decode_rle(AVCodecContext *avctx,
>> >>>TargaContext *s, const uint8_ *dst = *src;
>> >>>                  break;
>> >>>              case 2:
>> >>>-                *((uint16_t*)dst) = AV_RL16(src);
>> >>>+                *((uint16_t*)dst) = *((const uint16_t*)src);
>> >>>                  break;
>> >>>              case 3:
>> >>>                  dst[0] = src[0];
>> >>>@@ -78,7 +78,7 @@ static int targa_decode_rle(AVCodecContext *avctx,
>> >>>TargaContext *s, const uint8_ dst[2] = src[2];
>> >>>                  break;
>> >>>              case 4:
>> >>>-                *((uint32_t*)dst) = AV_RL32(src);
>> >>>+                *((uint32_t*)dst) = *((const uint32_t*)src);
>> >>
>> >>When changing it anyway, those might be better to use
>> >>AV_WN??A and AV_RN??A, it doesn't seem uglier to me than casts
>> >>and should reliably avoid aliasing issues (even if I can't imagine
>> >>a compiler ever doing something "wrong" here).
>> >
>> >As in attached?
>> >
>> >>Making the code using memcpy is an option, too, however I'd expect
>> >>depending on the content it will make the code anything from a
>> >>little faster to a whole lot slower.
>> >
>> >That was my original idea, but I now prefer the smaller change.
>
> Patch seems fine to me.
>
>>     case 15:
>> -        avctx->pix_fmt = PIX_FMT_RGB555;
>> +        avctx->pix_fmt = PIX_FMT_RGB555LE;
>>         break;
>>     case 16:
>> -        avctx->pix_fmt = PIX_FMT_RGB555;
>> +        avctx->pix_fmt = PIX_FMT_RGB555LE;
>>
>> nit: these two can be merged.
>
> True, though unrelated.
> I assume someone tested these are actually correct
> and the second one should not be 565?

Looks typo to me...