[FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
Use "palette side data" instead of "palette extradata" in error message.
--
Mats Peterson
http://matsp888.no-ip.org/~mats/
>From 962582ec4c265573a3230c089929854e1058c4af Mon Sep 17 00:00:00 2001
From: Mats Peterson
Date: Sun, 28 Feb 2016 05:25:36 +0100
Subject: [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
---
libavformat/avienc.c | 54 ++
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/libavformat/avienc.c b/libavformat/avienc.c
index ca505f4..9ac190a 100644
--- a/libavformat/avienc.c
+++ b/libavformat/avienc.c
@@ -668,39 +668,49 @@ static int avi_write_packet(AVFormatContext *s, AVPacket *pkt)
if ((ret = write_skip_frames(s, stream_index, pkt->dts)) < 0)
return ret;
-if (enc->codec_id == AV_CODEC_ID_RAWVIDEO && enc->codec_tag == 0) {
+if (enc->codec_id == AV_CODEC_ID_RAWVIDEO && enc->codec_tag == 0 && size) {
int64_t bpc = enc->bits_per_coded_sample != 15 ? enc->bits_per_coded_sample : 16;
int expected_stride = ((enc->width * bpc + 31) >> 5)*4;
-
+const uint8_t *pal = NULL;
+int pal_size = 1 << enc->bits_per_coded_sample;
+int pkt_pal_size, i;
ret = ff_reshuffle_raw_rgb(s, &pkt, enc, expected_stride);
if (ret < 0)
return ret;
-if (ret) {
-if (ret == CONTAINS_PAL) {
-int pc_tag, i;
-int pal_size = 1 << enc->bits_per_coded_sample;
-if (!avist->hdr_pal_done) {
-int64_t cur_offset = avio_tell(pb);
-avio_seek(pb, avist->pal_offset, SEEK_SET);
+pal = (uint8_t *)av_packet_get_side_data(pkt, AV_PKT_DATA_PALETTE,
+ &pkt_pal_size);
+if (pal && pkt_pal_size != AVPALETTE_SIZE) {
+av_log(s, AV_LOG_ERROR, "Invalid palette side data\n");
+return AVERROR_INVALIDDATA;
+}
+if (!pal && ret == CONTAINS_PAL)
+pal = data + size - AVPALETTE_SIZE;
+if (pal) {
+int pc_tag;
+if (!avist->hdr_pal_done) {
+int64_t cur_offset = avio_tell(pb);
+if (avio_seek(pb, avist->pal_offset, SEEK_SET) >= 0) {
for (i = 0; i < pal_size; i++) {
-uint32_t v = AV_RL32(data + size - 4*pal_size + 4*i);
+uint32_t v = AV_RL32(pal + 4*i);
avio_wl32(pb, v & 0xff);
}
avio_seek(pb, cur_offset, SEEK_SET);
-avist->hdr_pal_done++;
-}
-avi_stream2fourcc(tag, stream_index, enc->codec_type);
-tag[2] = 'p'; tag[3] = 'c';
-pc_tag = ff_start_tag(pb, tag);
-avio_w8(pb, 0);
-avio_w8(pb, pal_size & 0xFF);
-avio_wl16(pb, 0); // reserved
-for (i = 0; i < pal_size; i++) {
-uint32_t v = AV_RL32(data + size - 4*pal_size + 4*i);
-avio_wb32(pb, v<<8);
}
-ff_end_tag(pb, pc_tag);
+avist->hdr_pal_done++;
}
+avi_stream2fourcc(tag, stream_index, enc->codec_type);
+tag[2] = 'p'; tag[3] = 'c';
+pc_tag = ff_start_tag(pb, tag);
+avio_w8(pb, 0);
+avio_w8(pb, pal_size & 0xFF);
+avio_wl16(pb, 0); // reserved
+for (i = 0; i < pal_size; i++) {
+uint32_t v = AV_RL32(pal + 4*i);
+avio_wb32(pb, v<<8);
+}
+ff_end_tag(pb, pc_tag);
+}
+if (ret) {
ret = avi_write_packet_internal(s, pkt);
av_packet_free(&pkt);
return ret;
--
1.7.10.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On 02/28/2016 05:27 AM, Mats Peterson wrote: Use "palette side data" instead of "palette extradata" in error message. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Michael, I would like to add that ff_reshuffle_raw_rgb() that you wrote has the benefit of taking care of the correct stride when using stream copy as well. Thus, it won't be an "identical" stream copy, but it will be a valid one for the output file format in question. Mats -- Mats Peterson http://matsp888.no-ip.org/~mats/ ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On 02/28/2016 05:38 AM, Mats Peterson wrote: On 02/28/2016 05:27 AM, Mats Peterson wrote: Use "palette side data" instead of "palette extradata" in error message. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Michael, I would like to add that ff_reshuffle_raw_rgb() that you wrote has the benefit of taking care of the correct stride when using stream copy as well. Thus, it won't be an "identical" stream copy, but it will be a valid one for the output file format in question. Mats And Reimar, the AVI muxer libavformat/avienc.c uses *lots* of avio_seek() all over the place, so it won't work well on stdout regardless of my patch. I have checked for avio_seek() returning >= 0 in my part of the code in any case, but it won't make much of a difference. Mats -- Mats Peterson http://matsp888.no-ip.org/~mats/ ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On Sun, Feb 28, 2016 at 06:11:27AM +0100, Mats Peterson wrote: > On 02/28/2016 05:38 AM, Mats Peterson wrote: > >On 02/28/2016 05:27 AM, Mats Peterson wrote: > >>Use "palette side data" instead of "palette extradata" in error message. > >> > >> > >> > >>___ > >>ffmpeg-devel mailing list > >>ffmpeg-devel@ffmpeg.org > >>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > > > > > >Michael, I would like to add that ff_reshuffle_raw_rgb() that you wrote > >has the benefit of taking care of the correct stride when using stream > >copy as well. Thus, it won't be an "identical" stream copy, but it will > >be a valid one for the output file format in question. > > > >Mats > > > > And Reimar, the AVI muxer libavformat/avienc.c uses *lots* of avio_seek() > all over the place, so it won't work well on stdout regardless of my patch. > I have checked for avio_seek() returning >= 0 in my part of the code in any > case, but it won't make much of a difference. I think the effect of a failed avio_seek is kind of undefined, so checking the result is not really right as by then it's already broken. I also disagree with your analysis, all other avio_seek I could find are under if (pb->seekable), even if it is sometimes quite non-obvious. Also it might be helpful for you to have a look at tests/fate/demux.mak and tests/fate/video.mak and use copy-paste to write some basic tests for all the things you implement, so it doesn't get broken the moment you look the other way... ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On 02/28/2016 12:16 PM, Reimar Döffinger wrote: And Reimar, the AVI muxer libavformat/avienc.c uses *lots* of avio_seek() all over the place, so it won't work well on stdout regardless of my patch. I have checked for avio_seek() returning >= 0 in my part of the code in any case, but it won't make much of a difference. I think the effect of a failed avio_seek is kind of undefined, so checking the result is not really right as by then it's already broken. Well, the documentation says that avio_seek() is a variant of the fseek() function. I would rather say it's a variant of lseek(), since it returns the new position, not just 0 or -1. In any case, this is what the lseek() man page says: "On error, the value (off_t) -1 is returned and errno is set to indicate the error." So it's not really undefined. I also disagree with your analysis, all other avio_seek I could find are under if (pb->seekable), even if it is sometimes quite non-obvious. Yes, I've noticed that. Also it might be helpful for you to have a look at tests/fate/demux.mak and tests/fate/video.mak and use copy-paste to write some basic tests for all the things you implement, so it doesn't get broken the moment you look the other way... Yes, but I'm completely new at writing FATE tests, and it seems very complex to me, so I'll take a rain check on that one. Hopefully Michael will be able to add some good tests for the palette stuff, when he's got the time for it. Mats ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On 02/28/2016 12:26 PM, Mats Peterson wrote: On 02/28/2016 12:16 PM, Reimar Döffinger wrote: And Reimar, the AVI muxer libavformat/avienc.c uses *lots* of avio_seek() all over the place, so it won't work well on stdout regardless of my patch. I have checked for avio_seek() returning >= 0 in my part of the code in any case, but it won't make much of a difference. I think the effect of a failed avio_seek is kind of undefined, so checking the result is not really right as by then it's already broken. Well, the documentation says that avio_seek() is a variant of the fseek() function. I would rather say it's a variant of lseek(), since it returns the new position, not just 0 or -1. In any case, this is what the lseek() man page says: "On error, the value (off_t) -1 is returned and errno is set to indicate the error." So it's not really undefined. Even if the return value is not undefined on error from an avio_seek(), would you prefer using pb->seekable instead of invoking an avio_seek() function that will fail? Is it more elegant to you? Mats ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On Sun, Feb 28, 2016 at 12:33:16PM +0100, Mats Peterson wrote: > On 02/28/2016 12:26 PM, Mats Peterson wrote: > >On 02/28/2016 12:16 PM, Reimar Döffinger wrote: > >Well, the documentation says that avio_seek() is a variant of the > >fseek() function. I would rather say it's a variant of lseek(), since it > >returns the new position, not just 0 or -1. In any case, this is what > >the lseek() man page says: > > > >"On error, the value (off_t) -1 is returned and errno is set to > >indicate the error." > > > >So it's not really undefined. > > > > Even if the return value is not undefined on error from an avio_seek(), > would you prefer using pb->seekable instead of invoking an avio_seek() > function that will fail? Is it more elegant to you? I never said the return value was undefined, I said its behaviour (i.e. what it did to your stream) is undefined. E.g. it might have seeked back by the size of cached data instead of to the place you wanted. That's why checking pb->seekable is the only way fairly sure to not produce incorrect results. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On 02/28/2016 12:59 PM, Reimar Döffinger wrote: On Sun, Feb 28, 2016 at 12:33:16PM +0100, Mats Peterson wrote: On 02/28/2016 12:26 PM, Mats Peterson wrote: On 02/28/2016 12:16 PM, Reimar Döffinger wrote: Well, the documentation says that avio_seek() is a variant of the fseek() function. I would rather say it's a variant of lseek(), since it returns the new position, not just 0 or -1. In any case, this is what the lseek() man page says: "On error, the value (off_t) -1 is returned and errno is set to indicate the error." So it's not really undefined. Even if the return value is not undefined on error from an avio_seek(), would you prefer using pb->seekable instead of invoking an avio_seek() function that will fail? Is it more elegant to you? I never said the return value was undefined, I said its behaviour (i.e. what it did to your stream) is undefined. E.g. it might have seeked back by the size of cached data instead of to the place you wanted. That's why checking pb->seekable is the only way fairly sure to not produce incorrect results. ___ Alright. Sorry. Fixed in v4 1/2 v2 that I just posted. Have a look at it. Mats ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On Sun, Feb 28, 2016 at 12:26:15PM +0100, Mats Peterson wrote: > >Also it might be helpful for you to have a look at tests/fate/demux.mak > >and tests/fate/video.mak and use copy-paste to write some basic tests > >for all the things you implement, so it doesn't get broken the moment > >you look the other way... > > Yes, but I'm completely new at writing FATE tests, and it seems very complex > to me, so I'll take a rain check on that one. Hopefully Michael will be able > to add some good tests for the palette stuff, when he's got the time for it. That's why I said copy-paste. Either way there's little complex about it, they all just run a ffmpeg command (same as you do when developing tests, so you must know a few good ones) and then takes some form of hash/checksum of the result and stores that as reference. Sure some of the details are non-obvious (like where the input test files are stored) but what is in the mak files consists of some magic you generally don't need to understand and a FFmpeg command-line. With "make V=1 fate-testname" you see exactly what is being run. I may be wrong, but I think waiting for Michael to have "time over" so to say isn't a very successful strategy. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3 1/2] lavf/avienc: Add support for palette side data packets
On 02/28/2016 01:05 PM, Reimar Döffinger wrote: On Sun, Feb 28, 2016 at 12:26:15PM +0100, Mats Peterson wrote: Also it might be helpful for you to have a look at tests/fate/demux.mak and tests/fate/video.mak and use copy-paste to write some basic tests for all the things you implement, so it doesn't get broken the moment you look the other way... Yes, but I'm completely new at writing FATE tests, and it seems very complex to me, so I'll take a rain check on that one. Hopefully Michael will be able to add some good tests for the palette stuff, when he's got the time for it. That's why I said copy-paste. Either way there's little complex about it, they all just run a ffmpeg command (same as you do when developing tests, so you must know a few good ones) and then takes some form of hash/checksum of the result and stores that as reference. Sure some of the details are non-obvious (like where the input test files are stored) but what is in the mak files consists of some magic you generally don't need to understand and a FFmpeg command-line. With "make V=1 fate-testname" you see exactly what is being run. I may be wrong, but I think waiting for Michael to have "time over" so to say isn't a very successful strategy. ___ Perhaps not :) Anyway, that's a later issue. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
