Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Mats Peterson]

On 03/04/2016 03:51 AM, Mats Peterson wrote:

On 03/04/2016 03:49 AM, Michael Niedermayer wrote:

On Fri, Mar 04, 2016 at 03:41:23AM +0100, Mats Peterson wrote:

On 03/04/2016 03:30 AM, Michael Niedermayer wrote:


-if (!*palette && ret == CONTAINS_PAL)
-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
+if (!*palette && ret == CONTAINS_PAL) {
+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
+int i;
+for (i = 0; i < AVPALETTE_COUNT; i++) {
+uint8_t *p8 = pkt_pal + 4*i;



+uint32_t *p32 = (uint32_t *)p8;


undefined behavior, violating alignment requirements, and possibly
aliassing violation



Really? It works just fine, so please elaborate.


you cant cast arbitrary pointers to uint32_t

uint32_t can have platform specific alignment requirements that are
stricter than what a random 4 uint8_t have.
pkt->size does not need to be a multiple of 4 so even if pkt->data
would be aligned enough p8 doesnt have to be




Whatever. Forget this version 6 of the patch set and use version 5, with
version 2 of patch 4/4 in it (the one I posted in PM as well). And I
would be grateful if you could apply it soon. It's extensively tested here.

Mats



I get what you mean with the alignment issues, though.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Mats Peterson]

On 03/04/2016 03:49 AM, Michael Niedermayer wrote:

On Fri, Mar 04, 2016 at 03:41:23AM +0100, Mats Peterson wrote:

On 03/04/2016 03:30 AM, Michael Niedermayer wrote:


-if (!*palette && ret == CONTAINS_PAL)
-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
+if (!*palette && ret == CONTAINS_PAL) {
+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
+int i;
+for (i = 0; i < AVPALETTE_COUNT; i++) {
+uint8_t *p8 = pkt_pal + 4*i;



+uint32_t *p32 = (uint32_t *)p8;


undefined behavior, violating alignment requirements, and possibly
aliassing violation



Really? It works just fine, so please elaborate.


you cant cast arbitrary pointers to uint32_t

uint32_t can have platform specific alignment requirements that are
stricter than what a random 4 uint8_t have.
pkt->size does not need to be a multiple of 4 so even if pkt->data
would be aligned enough p8 doesnt have to be




Whatever. Forget this version 6 of the patch set and use version 5, with 
version 2 of patch 4/4 in it (the one I posted in PM as well). And I 
would be grateful if you could apply it soon. It's extensively tested here.


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Mats Peterson]

On 03/04/2016 03:45 AM, Mats Peterson wrote:

On 03/04/2016 03:41 AM, Mats Peterson wrote:

On 03/04/2016 03:30 AM, Michael Niedermayer wrote:


-if (!*palette && ret == CONTAINS_PAL)
-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
+if (!*palette && ret == CONTAINS_PAL) {
+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
+int i;
+for (i = 0; i < AVPALETTE_COUNT; i++) {
+uint8_t *p8 = pkt_pal + 4*i;



+uint32_t *p32 = (uint32_t *)p8;


undefined behavior, violating alignment requirements, and possibly
aliassing violation



Really? It works just fine, so please elaborate.




+*p32 = AV_RL32(p8);


corrupting potenially shared or read only input packet

[...]


That one I do understand.

___


Forget this version then, and use version 5. Just as long as it gets
applied in a foreseeable future.

Mats


-

Use version 5 with v2 of patch 4/4.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Michael Niedermayer]

On Fri, Mar 04, 2016 at 03:41:23AM +0100, Mats Peterson wrote:
> On 03/04/2016 03:30 AM, Michael Niedermayer wrote:
> >>
> >>-if (!*palette && ret == CONTAINS_PAL)
> >>-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
> >>+if (!*palette && ret == CONTAINS_PAL) {
> >>+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
> >>+int i;
> >>+for (i = 0; i < AVPALETTE_COUNT; i++) {
> >>+uint8_t *p8 = pkt_pal + 4*i;
> >
> >>+uint32_t *p32 = (uint32_t *)p8;
> >
> >undefined behavior, violating alignment requirements, and possibly
> >aliassing violation
> >
> 
> Really? It works just fine, so please elaborate.

you cant cast arbitrary pointers to uint32_t

uint32_t can have platform specific alignment requirements that are
stricter than what a random 4 uint8_t have.
pkt->size does not need to be a multiple of 4 so even if pkt->data
would be aligned enough p8 doesnt have to be


> 
> >
> >>+*p32 = AV_RL32(p8);
> >
> >corrupting potenially shared or read only input packet
> >
> >[...]
> 
> That one I do understand.
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Mats Peterson]

On 03/04/2016 03:41 AM, Mats Peterson wrote:

On 03/04/2016 03:30 AM, Michael Niedermayer wrote:


-if (!*palette && ret == CONTAINS_PAL)
-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
+if (!*palette && ret == CONTAINS_PAL) {
+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
+int i;
+for (i = 0; i < AVPALETTE_COUNT; i++) {
+uint8_t *p8 = pkt_pal + 4*i;



+uint32_t *p32 = (uint32_t *)p8;


undefined behavior, violating alignment requirements, and possibly
aliassing violation



Really? It works just fine, so please elaborate.




+*p32 = AV_RL32(p8);


corrupting potenially shared or read only input packet

[...]


That one I do understand.

___


Forget this version then, and use version 5. Just as long as it gets 
applied in a foreseeable future.


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Mats Peterson]

On 03/04/2016 03:30 AM, Michael Niedermayer wrote:


-if (!*palette && ret == CONTAINS_PAL)
-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
+if (!*palette && ret == CONTAINS_PAL) {
+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
+int i;
+for (i = 0; i < AVPALETTE_COUNT; i++) {
+uint8_t *p8 = pkt_pal + 4*i;



+uint32_t *p32 = (uint32_t *)p8;


undefined behavior, violating alignment requirements, and possibly
aliassing violation



Really? It works just fine, so please elaborate.




+*p32 = AV_RL32(p8);


corrupting potenially shared or read only input packet

[...]


That one I do understand.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Michael Niedermayer]

On Fri, Mar 04, 2016 at 03:08:48AM +0100, Mats Peterson wrote:
> 
> -- 
> Mats Peterson
> http://matsp888.no-ip.org/~mats/

>  internal.h |5 +++--
>  utils.c|   16 
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 10668c891a2fe1b1d311e6c48870ed3ef2967edc  
> 0004-lavf-utils-Normalize-AVPacket.data-to-native-endian.patch
> From 47d09853998e30e486b0069223a90d80e7742499 Mon Sep 17 00:00:00 2001
> From: Mats Peterson 
> Date: Fri, 4 Mar 2016 02:57:59 +0100
> Subject: [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian
> 
> ---
>  libavformat/internal.h |5 +++--
>  libavformat/utils.c|   16 
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 63e0632..fe95009 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -588,9 +588,10 @@ 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 pointer, which will be NULL if no palette found
>   */
> -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 85702dd..e66abcc 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4782,18 +4782,26 @@ int ff_standardize_creation_time(AVFormatContext *s)
>  return ret;
>  }
>  
> -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)
>  {
>  int size;
>  
> -*palette = av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE, );
> +*palette = (uint32_t *)av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE, 
> );
>  if (*palette && size != AVPALETTE_SIZE) {
>  av_log(s, AV_LOG_ERROR, "Invalid palette side data\n");
>  return AVERROR_INVALIDDATA;
>  }
>  
> -if (!*palette && ret == CONTAINS_PAL)
> -*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
> +if (!*palette && ret == CONTAINS_PAL) {
> +uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
> +int i;
> +for (i = 0; i < AVPALETTE_COUNT; i++) {
> +uint8_t *p8 = pkt_pal + 4*i;

> +uint32_t *p32 = (uint32_t *)p8;

undefined behavior, violating alignment requirements, and possibly
aliassing violation


> +*p32 = AV_RL32(p8);

corrupting potenially shared or read only input packet

[...]


-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

[Mats Peterson]

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
>From 47d09853998e30e486b0069223a90d80e7742499 Mon Sep 17 00:00:00 2001
From: Mats Peterson 
Date: Fri, 4 Mar 2016 02:57:59 +0100
Subject: [PATCH v6 4/4] lavf/utils: Normalize AVPacket.data to native endian

---
 libavformat/internal.h |5 +++--
 libavformat/utils.c|   16 
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/libavformat/internal.h b/libavformat/internal.h
index 63e0632..fe95009 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -588,9 +588,10 @@ 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 pointer, which will be NULL if no palette found
  */
-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 85702dd..e66abcc 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4782,18 +4782,26 @@ int ff_standardize_creation_time(AVFormatContext *s)
 return ret;
 }
 
-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)
 {
 int size;
 
-*palette = av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE, );
+*palette = (uint32_t *)av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE, );
 if (*palette && size != AVPALETTE_SIZE) {
 av_log(s, AV_LOG_ERROR, "Invalid palette side data\n");
 return AVERROR_INVALIDDATA;
 }
 
-if (!*palette && ret == CONTAINS_PAL)
-*palette = pkt->data + pkt->size - AVPALETTE_SIZE;
+if (!*palette && ret == CONTAINS_PAL) {
+uint8_t *pkt_pal = pkt->data + pkt->size - AVPALETTE_SIZE;
+int i;
+for (i = 0; i < AVPALETTE_COUNT; i++) {
+uint8_t *p8 = pkt_pal + 4*i;
+uint32_t *p32 = (uint32_t *)p8;
+*p32 = AV_RL32(p8);
+}
+*palette = (uint32_t *)pkt_pal;
+}
 
 return 0;
 }
-- 
1.7.10.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel