Re: [FFmpeg-devel] [PATCH 1/3 v2] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

On Tue, Nov 15, 2016 at 8:52 PM, James Almer  wrote:
> On 11/15/2016 10:39 PM, Michael Niedermayer wrote:
>> On Tue, Nov 15, 2016 at 11:56:48AM -0500, Vittorio Giovara wrote:
>> [...]
>>> +/**
>>> + * This structure describes how to handle spherical videos, outlining
>>> + * information about projection, initial layout, and any other view 
>>> modifier.
>>> + *
>>> + * @note The struct must be allocated with av_spherical_alloc() and
>>> + *   its size is not a part of the public ABI.
>>> + */
>>> +typedef struct AVSphericalMapping {
>>> +/**
>>> + * Projection type.
>>> + */
>>> +enum AVSphericalProjection projection;
>>> +
>>> +/**
>>> + * @name Initial orientation
>>> + * @{
>>> + * These fields represent the pose values that measure the rotation
>>> + * transformation (in degrees) to be applied to the projection.
>>
>>> + * They are exported as 16.16 fixed point.
>>
>> why waste 7 bits of precission ?
>
> 16.16 seems to be part of the spec
>
> https://github.com/google/spatial-media/blob/master/docs/spherical-video-v2-rfc.md

Correct, there is no point in adding more precision that the
underlying layer can withhold.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3 v2] mov: Export spherical information

[Vittorio Giovara]

On Tue, Nov 15, 2016 at 10:09 PM, James Almer  wrote:
> On 11/15/2016 1:56 PM, Vittorio Giovara wrote:
>> +static int mov_read_st3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +{
>> +AVStream *st;
>> +MOVStreamContext *sc;
>> +enum AVStereo3DType type;
>> +int mode;
>> +
>> +if (c->fc->nb_streams < 1)
>> +return 0;
>> +
>> +st = c->fc->streams[c->fc->nb_streams - 1];
>> +sc = st->priv_data;
>> +
>> +if (atom.size < 1) {
>> +av_log(c->fc, AV_LOG_ERROR, "Empty stereoscopic video box\n");
>> +return AVERROR_INVALIDDATA;
>> +}
>> +
>> +mode = avio_r8(pb);
>
> If i'm reading the spec right, st3d is a FullBox so before the data you'll
> have the box's version and flags (1 byte and 3 bytes respectively).

Oh I see, so I should check for version == 0 and read three bytes only
instead of 4.

>> +av_log(c->fc, AV_LOG_ERROR, "Missing projection header box\n");
>> +return 0;
>> +}
>> +
>> +/* 16.16 fixed point */
>> +yaw   = avio_rb32(pb);
>> +pitch = avio_rb32(pb);
>> +roll  = avio_rb32(pb);
>> +
>> +avio_skip(pb, size - 20);
>> +
>> +size = avio_rb32(pb);
>> +if (size > atom.size)
>> +return AVERROR_INVALIDDATA;
>> +
>> +tag = avio_rl32(pb);
>
> And all the possible cases for this one, except in here it seems to be 4 
> bytes for
> version and four for flags.
>
> Does Google offer samples to confirm this?

Yep.

https://transfer.sh/WOegj/motherboard-cube-v2-metadata.mp4
https://transfer.sh/31Zvg/motherboard-equirect-v1-metadata.mp4
https://transfer.sh/tn1xe/motherboard-equirect-v2-metadata.mp4

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

Re: [FFmpeg-devel] [PATCH] fate: Add h264 extradata reload tests

[Vittorio Giovara]

On Tue, Nov 15, 2016 at 8:47 PM, Michael Niedermayer
 wrote:
> On Tue, Nov 15, 2016 at 11:18:02AM -0500, Vittorio Giovara wrote:
>> ---
>> This is the version without hevc testing.
>> Please CC.
>> Vittorio
>>
>>  tests/fate/h264.mak  |  5 +
>>  tests/ref/fate/h264-extradata-reload | 13 +
>>  2 files changed, 18 insertions(+)
>>  create mode 100644 tests/ref/fate/h264-extradata-reload
>
> applied
>
> thx

Thanks! Do you think you could apply the hevc extradata reset patches
too (even without a test)?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/3 v2] mov: Export spherical information

[Vittorio Giovara]

This implements Spherical Video V1 and V2, as described in the
spatial-media collection by Google.

Signed-off-by: Vittorio Giovara 
---
Updated to use int32 for rotation.
Please CC.
Vittorio

 libavformat/isom.h |   7 ++
 libavformat/mov.c  | 281 -
 2 files changed, 287 insertions(+), 1 deletion(-)

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 02bfedd..0fd9eb0 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -24,6 +24,9 @@
 #ifndef AVFORMAT_ISOM_H
 #define AVFORMAT_ISOM_H
 
+#include "libavutil/spherical.h"
+#include "libavutil/stereo3d.h"
+
 #include "avio.h"
 #include "internal.h"
 #include "dv.h"
@@ -177,6 +180,10 @@ typedef struct MOVStreamContext {
 int stsd_count;
 
 int32_t *display_matrix;
+AVStereo3D *stereo3d;
+AVSphericalMapping *spherical;
+size_t spherical_size;
+
 uint32_t format;
 
 int has_sidx;  // If there is an sidx entry for this stream.
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6beb027..a017bc0 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -42,6 +42,8 @@
 #include "libavutil/aes.h"
 #include "libavutil/aes_ctr.h"
 #include "libavutil/sha.h"
+#include "libavutil/spherical.h"
+#include "libavutil/stereo3d.h"
 #include "libavutil/timecode.h"
 #include "libavcodec/ac3tab.h"
 #include "libavcodec/mpegaudiodecheader.h"
@@ -4497,8 +4499,230 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 return 0;
 }
 
+static int mov_read_st3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+AVStream *st;
+MOVStreamContext *sc;
+enum AVStereo3DType type;
+int mode;
+
+if (c->fc->nb_streams < 1)
+return 0;
+
+st = c->fc->streams[c->fc->nb_streams - 1];
+sc = st->priv_data;
+
+if (atom.size < 1) {
+av_log(c->fc, AV_LOG_ERROR, "Empty stereoscopic video box\n");
+return AVERROR_INVALIDDATA;
+}
+
+mode = avio_r8(pb);
+switch (mode) {
+case 0:
+type = AV_STEREO3D_2D;
+break;
+case 1:
+type = AV_STEREO3D_TOPBOTTOM;
+break;
+case 2:
+type = AV_STEREO3D_SIDEBYSIDE;
+break;
+default:
+av_log(c->fc, AV_LOG_WARNING, "Unknown st3d mode value %d\n", mode);
+return 0;
+}
+
+sc->stereo3d = av_stereo3d_alloc();
+if (!sc->stereo3d)
+return AVERROR(ENOMEM);
+
+sc->stereo3d->type = type;
+return 0;
+}
+
+static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+AVStream *st;
+MOVStreamContext *sc;
+int size;
+int32_t yaw, pitch, roll;
+uint32_t tag;
+unsigned l, t, r, b;
+enum AVSphericalProjection projection;
+
+if (c->fc->nb_streams < 1)
+return 0;
+
+st = c->fc->streams[c->fc->nb_streams - 1];
+sc = st->priv_data;
+
+if (atom.size < 4) {
+av_log(c->fc, AV_LOG_ERROR, "Empty spherical video box\n");
+return AVERROR_INVALIDDATA;
+}
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+if (tag != MKTAG('s','v','h','d')) {
+av_log(c->fc, AV_LOG_ERROR, "Missing spherical video header\n");
+return 0;
+}
+avio_skip(pb, size - 8); /* metadata_source */
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+if (tag != MKTAG('p','r','o','j')) {
+av_log(c->fc, AV_LOG_ERROR, "Missing projection box\n");
+return 0;
+}
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+if (tag != MKTAG('p','r','h','d')) {
+av_log(c->fc, AV_LOG_ERROR, "Missing projection header box\n");
+return 0;
+}
+
+/* 16.16 fixed point */
+yaw   = avio_rb32(pb);
+pitch = avio_rb32(pb);
+roll  = avio_rb32(pb);
+
+avio_skip(pb, size - 20);
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+switch (tag) {
+case MKTAG('c','b','m','p'):
+projection = AV_SPHERICAL_CUBEMAP;
+avio_skip(pb, 4); /* layout */
+l = t = r = b = avio_rb32(pb);
+break;
+case MKTAG('e','q','u','i'):
+projection = AV_SPHERICAL_EQUIRECTANGULAR;
+t = avio_rb32(pb);
+b = avio_rb32(pb);
+l = avio_rb32(pb);
+r = avio_rb32(pb);
+break;
+defaul

[FFmpeg-devel] [PATCH 1/3 v2] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

While no decoder currently exports spherical information, this type
represents a frame property that has to be passed through from container
to frames.

Signed-off-by: Vittorio Giovara 
---
Updated to use int32 for rotation.
Please CC.
Vittorio

 libavutil/Makefile|   2 +
 libavutil/avutil.h|   6 +++
 libavutil/frame.h |   8 ++-
 libavutil/spherical.c |  34 
 libavutil/spherical.h | 146 ++
 5 files changed, 195 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/spherical.c
 create mode 100644 libavutil/spherical.h

diff --git a/libavutil/Makefile b/libavutil/Makefile
index 0fa90fe..c5af79a 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -63,6 +63,7 @@ HEADERS = adler32.h   
  \
   samplefmt.h   \
   sha.h \
   sha512.h  \
+  spherical.h   \
   stereo3d.h\
   threadmessage.h   \
   time.h\
@@ -140,6 +141,7 @@ OBJS = adler32.o
\
samplefmt.o  \
sha.o\
sha512.o \
+   spherical.o  \
stereo3d.o   \
threadmessage.o  \
time.o   \
diff --git a/libavutil/avutil.h b/libavutil/avutil.h
index 29dd830..e9aaa03 100644
--- a/libavutil/avutil.h
+++ b/libavutil/avutil.h
@@ -118,6 +118,12 @@
  *
  * @}
  *
+ * @defgroup lavu_video Video related
+ *
+ * @{
+ *
+ * @}
+ *
  * @defgroup lavu_audio Audio related
  *
  * @{
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 8e51361..b450092 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -120,7 +120,13 @@ enum AVFrameSideDataType {
  * The GOP timecode in 25 bit timecode format. Data format is 64-bit 
integer.
  * This is set on the first frame of a GOP that has a temporal reference 
of 0.
  */
-AV_FRAME_DATA_GOP_TIMECODE
+AV_FRAME_DATA_GOP_TIMECODE,
+
+/**
+ * The data represents the AVSphericalMapping structure defined in
+ * libavutil/spherical.h.
+ */
+AV_FRAME_DATA_SPHERICAL,
 };
 
 enum AVActiveFormatDescription {
diff --git a/libavutil/spherical.c b/libavutil/spherical.c
new file mode 100644
index 000..816452c
--- /dev/null
+++ b/libavutil/spherical.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2016 Vittorio Giovara 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "mem.h"
+#include "spherical.h"
+
+AVSphericalMapping *av_spherical_alloc(size_t *size)
+{
+AVSphericalMapping *spherical = av_mallocz(sizeof(AVSphericalMapping));
+if (!spherical)
+return NULL;
+
+if (size)
+*size = sizeof(*spherical);
+
+return spherical;
+}
diff --git a/libavutil/spherical.h b/libavutil/spherical.h
new file mode 100644
index 000..6d5a316
--- /dev/null
+++ b/libavutil/spherical.h
@@ -0,0 +1,146 @@
+/*
+ * Copyright (c) 2016 Vittorio Giovara 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU L

[FFmpeg-devel] [PATCH 2/3 v2] lavc: Add spherical packet side data API

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
Updated to use int32 for rotation.
Please CC.
Vittorio

 ffprobe.c | 18 ++
 libavcodec/avcodec.h  |  8 +++-
 libavcodec/avpacket.c |  1 +
 libavcodec/utils.c|  1 +
 libavformat/dump.c| 36 
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/ffprobe.c b/ffprobe.c
index a2980b3..839963a 100644
--- a/ffprobe.c
+++ b/ffprobe.c
@@ -37,6 +37,7 @@
 #include "libavutil/hash.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
+#include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
 #include "libavutil/dict.h"
 #include "libavutil/intreadwrite.h"
@@ -1783,6 +1784,23 @@ static void print_pkt_side_data(WriterContext *w,
 const AVStereo3D *stereo = (AVStereo3D *)sd->data;
 print_str("type", av_stereo3d_type_name(stereo->type));
 print_int("inverted", !!(stereo->flags & AV_STEREO3D_FLAG_INVERT));
+} else if (sd->type == AV_PKT_DATA_SPHERICAL) {
+const AVSphericalMapping *spherical = (AVSphericalMapping 
*)sd->data;
+if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR)
+print_str("projection", "equirectangular");
+else if (spherical->projection == AV_SPHERICAL_CUBEMAP)
+print_str("projection", "cubemap");
+else
+print_str("projection", "unknown");
+
+print_int("yaw", ((double)spherical->yaw) / (1 << 16));
+print_int("pitch", ((double)spherical->pitch) / (1 << 16));
+print_int("roll", ((double)spherical->roll) / (1 << 16));
+
+print_int("left", spherical->left_offset);
+print_int("top", spherical->top_offset);
+print_int("right", spherical->right_offset);
+print_int("bottom", spherical->bottom_offset);
 }
 writer_print_section_footer(w);
 }
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 22f..9497cc3 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1536,7 +1536,13 @@ enum AVPacketSideDataType {
  * should be associated with a video stream and containts data in the form
  * of the AVMasteringDisplayMetadata struct.
  */
-AV_PKT_DATA_MASTERING_DISPLAY_METADATA
+AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
+
+/**
+ * This side data should be associated with a video stream and corresponds
+ * to the AVSphericalMapping structure.
+ */
+AV_PKT_DATA_SPHERICAL,
 };
 
 #define AV_PKT_DATA_QUALITY_FACTOR AV_PKT_DATA_QUALITY_STATS //DEPRECATED
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index c3f871c..a799610 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -371,6 +371,7 @@ const char *av_packet_side_data_name(enum 
AVPacketSideDataType type)
 case AV_PKT_DATA_METADATA_UPDATE:return "Metadata Update";
 case AV_PKT_DATA_MPEGTS_STREAM_ID:   return "MPEGTS Stream ID";
 case AV_PKT_DATA_MASTERING_DISPLAY_METADATA: return "Mastering display 
metadata";
+case AV_PKT_DATA_SPHERICAL:  return "Spherical mapping";
 }
 return NULL;
 }
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index d6dca18..89a12c6 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -762,6 +762,7 @@ int ff_init_buffer_info(AVCodecContext *avctx, AVFrame 
*frame)
 } sd[] = {
 { AV_PKT_DATA_REPLAYGAIN ,AV_FRAME_DATA_REPLAYGAIN },
 { AV_PKT_DATA_DISPLAYMATRIX,  AV_FRAME_DATA_DISPLAYMATRIX 
},
+{ AV_PKT_DATA_SPHERICAL,  AV_FRAME_DATA_SPHERICAL },
 { AV_PKT_DATA_STEREO3D,   AV_FRAME_DATA_STEREO3D },
 { AV_PKT_DATA_AUDIO_SERVICE_TYPE, 
AV_FRAME_DATA_AUDIO_SERVICE_TYPE },
 { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, 
AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
diff --git a/libavformat/dump.c b/libavformat/dump.c
index cd14625..2dd7a0a 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -31,6 +31,7 @@
 #include "libavutil/opt.h"
 #include "libavutil/avstring.h"
 #include "libavutil/replaygain.h"
+#include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
 
 #include "avformat.h"
@@ -342,6 +343,37 @@ static void dump_mastering_display_metadata(void *ctx, 
AVPacketSideData* sd) {
av_q2d(metadata->min_luminance), av_q2d(metadata->max_luminance));
 }
 
+static void dump_spherical(void *ctx, AVPacketSideData *sd)
+{
+AVSphericalMapping *spherical = (AVSphericalMapping *)sd->

[FFmpeg-devel] [PATCHv5] mov: Evaluate the movie display matrix

[Vittorio Giovara]

This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet. So store it in main context, and apply
it when appropriate, that is after parsing the tkhd one.

Signed-off-by: Vittorio Giovara 
---
Fixed a boolean operation. No other changes, ready to be committed.
Please CC.
Vittorio

 libavformat/isom.h   |  1 +
 libavformat/mov.c| 48 +---
 tests/fate/mov.mak   |  5 -
 tests/ref/fate/mov-displaymatrix | 10 +
 4 files changed, 50 insertions(+), 14 deletions(-)
 create mode 100644 tests/ref/fate/mov-displaymatrix

diff --git a/libavformat/isom.h b/libavformat/isom.h
index d684502..02bfedd 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -240,6 +240,7 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 8d6cc12..b7d0b12 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1239,6 +1239,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1269,7 +1270,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3847,12 +3853,22 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 return 0;
 }
 
+// return 1 when matrix is identity, 0 otherwise
+#define IS_MATRIX_IDENT(matrix)\
+( (matrix)[0][0] == (1 << 16) &&   \
+  (matrix)[1][1] == (1 << 16) &&   \
+  (matrix)[2][2] == (1 << 30) &&   \
+ !(matrix)[0][1] && !(matrix)[0][2] && \
+ !(matrix)[1][0] && !(matrix)[1][2] && \
+ !(matrix)[2][0] && !(matrix)[2][1])
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
+int res_display_matrix[3][3] = { { 0 } };
 AVStream *st;
 MOVStreamContext *sc;
 int version;
@@ -3902,15 +3918,20 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 sc->width = width >> 16;
 sc->height = height >> 16;
 
-// save the matrix and add rotate metadata when it is not the default
-// identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+// apply the moov display matrix (after the tkhd one)
+for (i = 0; i < 3; i++) {
+const int sh[3] = { 16, 16, 30 };
+for (j = 0; j < 3; j++) {
+for (e = 0; e < 3; e++) {
+res_display_matrix[i][j] +=
+((int64_t) display_matrix[i][e] *
+ c->movie_display_matrix[e][j]) >> sh[e];
+}
+}
+}
+
+// save the matrix when it is not the default identity
+if (!IS_MATRIX_IDENT(res_display_matrix)) {
 double rotate;
 
 av_freep(&sc->display_matrix);
@@ -3920,7 +3941,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 for (i = 0; i < 3; i++)
 for (j = 0; j < 3; j++)
-sc->display_matrix[i * 3 + j] = display_matrix[i][j];
+sc->display_matrix[i * 3 + j] = res_display_matrix[i][j];
 
 rotate = av_display_rotation_get(sc->display_matrix);
 if (!isnan(rotate)) {
@@ -3939,7 +3960,8 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 double disp_transform[2];
 
 for (i = 0; i < 2; i++)
-disp_transform[i] = hypot(display_matrix[i][0], 
display_matrix[i][1]);
+disp_transform[i] = hypot(sc->display_matrix[0 + i],
+

[FFmpeg-devel] [PATCH] fate: Add h264 extradata reload tests

[Vittorio Giovara]

---
This is the version without hevc testing.
Please CC.
Vittorio

 tests/fate/h264.mak  |  5 +
 tests/ref/fate/h264-extradata-reload | 13 +
 2 files changed, 18 insertions(+)
 create mode 100644 tests/ref/fate/h264-extradata-reload

diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak
index b4d7f7a..f8ef3f4 100644
--- a/tests/fate/h264.mak
+++ b/tests/fate/h264.mak
@@ -196,6 +196,10 @@ FATE_H264  := $(FATE_H264:%=fate-h264-conformance-%)   
 \
 
 FATE_H264-$(call DEMDEC, H264, H264) += $(FATE_H264)
 FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-crop-to-container
+
+# this sample has two stsd entries and needs to reload extradata
+FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-extradata-reload
+
 FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-interlace-crop
 
 # this sample has invalid reference list modification, but decodes fine
@@ -408,6 +412,7 @@ fate-h264-conformance-sva_nl2_e:  CMD = 
framecrc -vsync drop -i
 fate-h264-bsf-mp4toannexb:CMD = md5 -i 
$(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vcodec copy -f h264
 
 fate-h264-crop-to-container:  CMD = framemd5 -i 
$(TARGET_SAMPLES)/h264/crop-to-container-dims-canon.mov
+fate-h264-extradata-reload:   CMD = framemd5 -i 
$(TARGET_SAMPLES)/h264/extradata-reload-multi-stsd.mov
 fate-h264-extreme-plane-pred: CMD = framemd5 -i 
$(TARGET_SAMPLES)/h264/extreme-plane-pred.h264
 fate-h264-interlace-crop: CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vframes 3
 fate-h264-lossless:   CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/lossless.h264
diff --git a/tests/ref/fate/h264-extradata-reload 
b/tests/ref/fate/h264-extradata-reload
new file mode 100644
index 000..c70f805
--- /dev/null
+++ b/tests/ref/fate/h264-extradata-reload
@@ -0,0 +1,13 @@
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 256x128
+#sar 0: 1/1
+#stream#, dts,pts, duration, size, hash
+0,  0,  0,1,49152, ae09c88e87e3ea0aa8ad267ee91222e5
+0,  1,  1,1,49152, 7ccd8321a6e23ae1f82bd323c8376524
+0,  2,  2,1,49152, 8ed93ab585ff9848801cc28b75b5e12d
+0,  3,  3,1,49152, 0049913870ddb62ffc535282018766f4
-- 
2.10.0

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

On Mon, Nov 14, 2016 at 8:55 PM, Michael Niedermayer
 wrote:
> On Mon, Nov 14, 2016 at 04:55:36PM -0500, Vittorio Giovara wrote:
>> On Sun, Nov 13, 2016 at 11:57 AM, Michael Niedermayer
>>  wrote:
>> > On Sun, Nov 13, 2016 at 10:18:18AM +0100, Michael Niedermayer wrote:
>> >> On Sat, Nov 12, 2016 at 10:05:22PM -0500, Vittorio Giovara wrote:
>> >> [...]
>> >> > So I really do not see a use case for using int64 here.
>> >>
>> >> then you can use int32, less than int32 is too little if for example
>> >> you wnat the precission to be sufficient to know where a tele lens
>> >> points with pixel precission and have a bit precission headroom
>>
>> Hi Michael, thanks for keeping me in CC.
>> I understand the problem, but this is a solution for a issue that is
>> non-existent.
>
> The problem of difficut to test code is real and existing in general.
> Encoders&muxers using floats/doubles can not be tested as completely as
> Encoders&muxers not using floats unless they by chance give binary
> identical results on all platforms.

That's not the problem you referred to in the previous email.

> Muxers&encoders would use/write the rotation side data
>
> Similarly ffprobe would at some point print this data, the text
> printout of doubles has a good chance to not match exactly between
> platforms.

ffprobe does that in 2/3, but right now it's limited to int.

>> There is no application or usecase for "pixel perfect"
>> precision, the rendering of a spherical video will distort the video
>> surface in order to map the flat surface to a sphere, and it is
>> impossible to predict where this operation will move pixels to. I
>> believe this is why this specification describes the initial
>> orientation as rotation degrees rather than pixel offsets.
>
> I referred to pixels only to show that 32bit floats are insufficiently
> precisse in some cases.

I still don't like it, but I'll just export the original 16.16 fixed
point value and let the user deal with it then.
Thanks for the reviews.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

On Sun, Nov 13, 2016 at 11:57 AM, Michael Niedermayer
 wrote:
> On Sun, Nov 13, 2016 at 10:18:18AM +0100, Michael Niedermayer wrote:
>> On Sat, Nov 12, 2016 at 10:05:22PM -0500, Vittorio Giovara wrote:
>> [...]
>> > So I really do not see a use case for using int64 here.
>>
>> then you can use int32, less than int32 is too little if for example
>> you wnat the precission to be sufficient to know where a tele lens
>> points with pixel precission and have a bit precission headroom

Hi Michael, thanks for keeping me in CC.
I understand the problem, but this is a solution for a issue that is
non-existent. There is no application or usecase for "pixel perfect"
precision, the rendering of a spherical video will distort the video
surface in order to map the flat surface to a sphere, and it is
impossible to predict where this operation will move pixels to. I
believe this is why this specification describes the initial
orientation as rotation degrees rather than pixel offsets.

As I said, I believe the use case of a rotation calls in the use of
double, exactly as it happens for other rotation APIs in the codebase.
Secondly I am against exporting the data as fixed point since that
would be burden to the end user and would tie this API to the google
specification too much. Finally I don't see what is wrong about these
fields being platform-specific, the whole structure is bound to be
platform specific, and the API mandates that a structure is used all
the time.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

On Sat, Nov 12, 2016 at 6:11 PM, Michael Niedermayer
 wrote:
> Hi
>
> On Sat, Nov 12, 2016 at 12:30:52PM -0500, Vittorio Giovara wrote:
>> On Sat, Nov 12, 2016 at 9:41 AM, James Almer  wrote:
>> > On 11/11/2016 10:39 PM, Michael Niedermayer wrote:
>> >> On Fri, Nov 11, 2016 at 05:49:00PM -0500, Vittorio Giovara wrote:
> [...]
>> >>
>> >>> +double yaw;   ///< Clockwise rotation around the up vector [-180, 
>> >>> 180].
>> >>> +double pitch; ///< Counter-clockwise rotation around the right 
>> >>> vector [-90, 90].
>> >>> +double roll;  ///< Counter-clockwise rotation around the forward 
>> >>> vector [-180, 180].
>> >>
>> >> please use intXY (64 or 32 as preferred) so there are no platform
>> >> rounding dependancies
>>
>> These are rotation angles which are inherently floating point, and
>
> Theres nothing inherently floating point on an angle.

Err not sure where you get that from, but it is very common for
rotation to be represented in floating point, whether you express it
in degrees (and thus real numbers, which include fractional values
such as 90.5º, expressed as floating point in computing) and in
radians where you literally use PI (which is an irrational numerical
constant, again expressed as floating point in computing).

>> consistent with what other rotation-related APIs export (eg.
>> av_display_matrix_rotation_get()).
>
>> Besides using intXX would lose
>> precision that the original specification offers.
>
> int64_t has about a thousand times higher precission than a double
> for storing general -180°..+180° angles. This is because doubles have
> 11 bits for a exponent which is exactly the same value for most of the
> angles

How much precision is too precision? double values are extremely well
suited for the kind of angles they have to represent: 11 bits are way
more than enough to express values between 180 and -180, and the rest
can be used for the mantissa (bearing a value between 0 and
0.99..) good enough.

So I really do not see a use case for using int64 here.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

On Sat, Nov 12, 2016 at 9:41 AM, James Almer  wrote:
> On 11/11/2016 10:39 PM, Michael Niedermayer wrote:
>> On Fri, Nov 11, 2016 at 05:49:00PM -0500, Vittorio Giovara wrote:
>> [...]
>>> +/**
>>> + * This structure describes how to handle spherical videos, outlining
>>> + * information about projection, initial layout, and any other view 
>>> modifier.
>>> + *
>>> + * @note The struct must be allocated with av_spherical_alloc() and
>>> + *   its size is not a part of the public ABI.
>>> + */
>>> +typedef struct AVSphericalMapping {
>>> +/**
>>> + * Projection type.
>>> + */
>>> +enum AVSphericalProjection projection;
>>> +
>>> +/**
>>> + * @name Initial orientation
>>> + * @{
>>> + * These fields represent the pose values that measure the rotation
>>> + * transformation (in degrees) to be applied to the projection.
>>> + * See this equirectangular projection as example:
>>> + *
>>> + * @code{.unparsed}
>>> + *   Yaw
>>> + * -180   0   180
>>> + *   90 +-+-+  180
>>> + *  | | |
>>> + * P| |  o> |
>>> + * i| ^ |
>>> + * t  0 +-X-+0 Roll
>>> + * c| | |
>>> + * h| | |
>>> + *  | | |
>>> + *  -90 +-+-+ -180
>>> + *
>>> + * X - the default camera center
>>> + * ^ - the default up vector
>>> + * > - the up vector for a rotation of 90 degrees
>>> + * o - the image center for yaw = 90, pitch = 45, roll = 0
>>> + * @endcode
>>> + *
>>> + * The order of transformation is always yaw, followed by pitch, and
>>> + * finally by roll.
>>> + */
>>
>>> +double yaw;   ///< Clockwise rotation around the up vector [-180, 180].
>>> +double pitch; ///< Counter-clockwise rotation around the right vector 
>>> [-90, 90].
>>> +double roll;  ///< Counter-clockwise rotation around the forward 
>>> vector [-180, 180].
>>
>> please use intXY (64 or 32 as preferred) so there are no platform
>> rounding dependancies

These are rotation angles which are inherently floating point, and
consistent with what other rotation-related APIs export (eg.
av_display_matrix_rotation_get()). Besides using intXX would lose
precision that the original specification offers.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] mov: Export spherical information

[Vittorio Giovara]

On Fri, Nov 11, 2016 at 6:23 PM, James Almer  wrote:
> On 11/11/2016 7:49 PM, Vittorio Giovara wrote:
>> This implements Spherical Video V1 and V2, as described in the
>> spatial-media collection by Google.
>>
>> Signed-off-by: Vittorio Giovara 
>> ---
>> Please CC.
>> Vittorio
>>
>>  libavformat/isom.h |   7 ++
>>  libavformat/mov.c  | 281 
>> -
>>  2 files changed, 287 insertions(+), 1 deletion(-)
>
> [...]
>
>> @@ -5682,6 +5927,40 @@ static int mov_read_header(AVFormatContext *s)
>>  sd->data = (uint8_t*)sc->display_matrix;
>>  sc->display_matrix = NULL;
>>  }
>> +if (sc->stereo3d) {
>> +AVPacketSideData *sd, *tmp;
>> +
>> +tmp = av_realloc_array(st->side_data,
>> +   st->nb_side_data + 1, sizeof(*tmp));
>> +if (!tmp)
>> +return AVERROR(ENOMEM);
>> +
>> +st->side_data = tmp;
>> +st->nb_side_data++;
>> +
>> +sd = &st->side_data[st->nb_side_data - 1];
>> +sd->type = AV_PKT_DATA_STEREO3D;
>> +sd->size = sizeof(*sc->stereo3d);
>> +sd->data = (uint8_t *)sc->stereo3d;
>> +sc->stereo3d = NULL;
>> +}
>> +if (sc->spherical) {
>> +AVPacketSideData *sd, *tmp;
>> +
>> +tmp = av_realloc_array(st->side_data,
>> +   st->nb_side_data + 1, sizeof(*tmp));
>> +if (!tmp)
>> +return AVERROR(ENOMEM);
>> +
>> +st->side_data = tmp;
>> +st->nb_side_data++;
>> +
>> +sd = &st->side_data[st->nb_side_data - 1];
>> +sd->type = AV_PKT_DATA_SPHERICAL;
>> +sd->size = sc->spherical_size;
>> +sd->data = (uint8_t *)sc->spherical;
>> +sc->spherical = NULL;
>> +}
>
> Why isn't this using av_stream_new_side_data()?

I didn't want to mix refactors and new code in a single patch,
coalescing that portion of code may be done later.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/3] mov: Export spherical information

[Vittorio Giovara]

This implements Spherical Video V1 and V2, as described in the
spatial-media collection by Google.

Signed-off-by: Vittorio Giovara 
---
Please CC.
Vittorio

 libavformat/isom.h |   7 ++
 libavformat/mov.c  | 281 -
 2 files changed, 287 insertions(+), 1 deletion(-)

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 02bfedd..0fd9eb0 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -24,6 +24,9 @@
 #ifndef AVFORMAT_ISOM_H
 #define AVFORMAT_ISOM_H
 
+#include "libavutil/spherical.h"
+#include "libavutil/stereo3d.h"
+
 #include "avio.h"
 #include "internal.h"
 #include "dv.h"
@@ -177,6 +180,10 @@ typedef struct MOVStreamContext {
 int stsd_count;
 
 int32_t *display_matrix;
+AVStereo3D *stereo3d;
+AVSphericalMapping *spherical;
+size_t spherical_size;
+
 uint32_t format;
 
 int has_sidx;  // If there is an sidx entry for this stream.
diff --git a/libavformat/mov.c b/libavformat/mov.c
index cb3c61c..d36c935 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -42,6 +42,8 @@
 #include "libavutil/aes.h"
 #include "libavutil/aes_ctr.h"
 #include "libavutil/sha.h"
+#include "libavutil/spherical.h"
+#include "libavutil/stereo3d.h"
 #include "libavutil/timecode.h"
 #include "libavcodec/ac3tab.h"
 #include "libavcodec/mpegaudiodecheader.h"
@@ -4498,8 +4500,230 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 return 0;
 }
 
+static int mov_read_st3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+AVStream *st;
+MOVStreamContext *sc;
+enum AVStereo3DType type;
+int mode;
+
+if (c->fc->nb_streams < 1)
+return 0;
+
+st = c->fc->streams[c->fc->nb_streams - 1];
+sc = st->priv_data;
+
+if (atom.size < 1) {
+av_log(c->fc, AV_LOG_ERROR, "Empty stereoscopic video box\n");
+return AVERROR_INVALIDDATA;
+}
+
+mode = avio_r8(pb);
+switch (mode) {
+case 0:
+type = AV_STEREO3D_2D;
+break;
+case 1:
+type = AV_STEREO3D_TOPBOTTOM;
+break;
+case 2:
+type = AV_STEREO3D_SIDEBYSIDE;
+break;
+default:
+av_log(c->fc, AV_LOG_WARNING, "Unknown st3d mode value %d\n", mode);
+return 0;
+}
+
+sc->stereo3d = av_stereo3d_alloc();
+if (!sc->stereo3d)
+return AVERROR(ENOMEM);
+
+sc->stereo3d->type = type;
+return 0;
+}
+
+static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+AVStream *st;
+MOVStreamContext *sc;
+int size;
+int32_t yaw, pitch, roll;
+uint32_t tag;
+unsigned l, t, r, b;
+enum AVSphericalProjection projection;
+
+if (c->fc->nb_streams < 1)
+return 0;
+
+st = c->fc->streams[c->fc->nb_streams - 1];
+sc = st->priv_data;
+
+if (atom.size < 4) {
+av_log(c->fc, AV_LOG_ERROR, "Empty spherical video box\n");
+return AVERROR_INVALIDDATA;
+}
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+if (tag != MKTAG('s','v','h','d')) {
+av_log(c->fc, AV_LOG_ERROR, "Missing spherical video header\n");
+return 0;
+}
+avio_skip(pb, size - 8); /* metadata_source */
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+if (tag != MKTAG('p','r','o','j')) {
+av_log(c->fc, AV_LOG_ERROR, "Missing projection box\n");
+return 0;
+}
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+if (tag != MKTAG('p','r','h','d')) {
+av_log(c->fc, AV_LOG_ERROR, "Missing projection header box\n");
+return 0;
+}
+
+/* 16.16 fixed point */
+yaw   = avio_rb32(pb);
+pitch = avio_rb32(pb);
+roll  = avio_rb32(pb);
+
+avio_skip(pb, size - 20);
+
+size = avio_rb32(pb);
+if (size > atom.size)
+return AVERROR_INVALIDDATA;
+
+tag = avio_rl32(pb);
+switch (tag) {
+case MKTAG('c','b','m','p'):
+projection = AV_SPHERICAL_CUBEMAP;
+avio_skip(pb, 4); /* layout */
+l = t = r = b = avio_rb32(pb);
+break;
+case MKTAG('e','q','u','i'):
+projection = AV_SPHERICAL_EQUIRECTANGULAR;
+t = avio_rb32(pb);
+b = avio_rb32(pb);
+l = avio_rb32(pb);
+r = avio_rb32(pb);
+break;
+default:
+av_log(c->fc, AV_L

[FFmpeg-devel] [PATCH 1/3] lavu: Add AVSphericalMapping type and frame side data

[Vittorio Giovara]

While no decoder currently exports spherical information, this type
represents a frame property that has to be passed through from container
to frames.

Signed-off-by: Vittorio Giovara 
---
Missing version bump (do so when pushing).
Please CC.
Vittorio

 libavutil/Makefile|   2 +
 libavutil/avutil.h|   6 +++
 libavutil/frame.h |   8 ++-
 libavutil/spherical.c |  34 
 libavutil/spherical.h | 144 ++
 5 files changed, 193 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/spherical.c
 create mode 100644 libavutil/spherical.h

diff --git a/libavutil/Makefile b/libavutil/Makefile
index 0fa90fe..c5af79a 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -63,6 +63,7 @@ HEADERS = adler32.h   
  \
   samplefmt.h   \
   sha.h \
   sha512.h  \
+  spherical.h   \
   stereo3d.h\
   threadmessage.h   \
   time.h\
@@ -140,6 +141,7 @@ OBJS = adler32.o
\
samplefmt.o  \
sha.o\
sha512.o \
+   spherical.o  \
stereo3d.o   \
threadmessage.o  \
time.o   \
diff --git a/libavutil/avutil.h b/libavutil/avutil.h
index 29dd830..e9aaa03 100644
--- a/libavutil/avutil.h
+++ b/libavutil/avutil.h
@@ -118,6 +118,12 @@
  *
  * @}
  *
+ * @defgroup lavu_video Video related
+ *
+ * @{
+ *
+ * @}
+ *
  * @defgroup lavu_audio Audio related
  *
  * @{
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 8e51361..b450092 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -120,7 +120,13 @@ enum AVFrameSideDataType {
  * The GOP timecode in 25 bit timecode format. Data format is 64-bit 
integer.
  * This is set on the first frame of a GOP that has a temporal reference 
of 0.
  */
-AV_FRAME_DATA_GOP_TIMECODE
+AV_FRAME_DATA_GOP_TIMECODE,
+
+/**
+ * The data represents the AVSphericalMapping structure defined in
+ * libavutil/spherical.h.
+ */
+AV_FRAME_DATA_SPHERICAL,
 };
 
 enum AVActiveFormatDescription {
diff --git a/libavutil/spherical.c b/libavutil/spherical.c
new file mode 100644
index 000..816452c
--- /dev/null
+++ b/libavutil/spherical.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2016 Vittorio Giovara 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "mem.h"
+#include "spherical.h"
+
+AVSphericalMapping *av_spherical_alloc(size_t *size)
+{
+AVSphericalMapping *spherical = av_mallocz(sizeof(AVSphericalMapping));
+if (!spherical)
+return NULL;
+
+if (size)
+*size = sizeof(*spherical);
+
+return spherical;
+}
diff --git a/libavutil/spherical.h b/libavutil/spherical.h
new file mode 100644
index 000..57cd36b
--- /dev/null
+++ b/libavutil/spherical.h
@@ -0,0 +1,144 @@
+/*
+ * Copyright (c) 2016 Vittorio Giovara 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of th

[FFmpeg-devel] [PATCH 2/3] lavc: Add spherical packet side data API

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
Missing version bump (do so when pushing).
Please CC.
Vittorio

 ffprobe.c | 18 ++
 libavcodec/avcodec.h  |  8 +++-
 libavcodec/avpacket.c |  1 +
 libavcodec/utils.c|  1 +
 libavformat/dump.c| 33 +
 5 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/ffprobe.c b/ffprobe.c
index a2980b3..e89e5d9 100644
--- a/ffprobe.c
+++ b/ffprobe.c
@@ -37,6 +37,7 @@
 #include "libavutil/hash.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
+#include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
 #include "libavutil/dict.h"
 #include "libavutil/intreadwrite.h"
@@ -1783,6 +1784,23 @@ static void print_pkt_side_data(WriterContext *w,
 const AVStereo3D *stereo = (AVStereo3D *)sd->data;
 print_str("type", av_stereo3d_type_name(stereo->type));
 print_int("inverted", !!(stereo->flags & AV_STEREO3D_FLAG_INVERT));
+} else if (sd->type == AV_PKT_DATA_SPHERICAL) {
+const AVSphericalMapping *spherical = (AVSphericalMapping 
*)sd->data;
+if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR)
+print_str("projection", "equirectangular");
+else if (spherical->projection == AV_SPHERICAL_CUBEMAP)
+print_str("projection", "cubemap");
+else
+print_str("projection", "unknown");
+
+print_int("yaw", spherical->yaw);
+print_int("pitch", spherical->pitch);
+print_int("roll", spherical->roll);
+
+print_int("left", spherical->left_offset);
+print_int("top", spherical->top_offset);
+print_int("right", spherical->right_offset);
+print_int("bottom", spherical->bottom_offset);
 }
 writer_print_section_footer(w);
 }
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 22f..9497cc3 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1536,7 +1536,13 @@ enum AVPacketSideDataType {
  * should be associated with a video stream and containts data in the form
  * of the AVMasteringDisplayMetadata struct.
  */
-AV_PKT_DATA_MASTERING_DISPLAY_METADATA
+AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
+
+/**
+ * This side data should be associated with a video stream and corresponds
+ * to the AVSphericalMapping structure.
+ */
+AV_PKT_DATA_SPHERICAL,
 };
 
 #define AV_PKT_DATA_QUALITY_FACTOR AV_PKT_DATA_QUALITY_STATS //DEPRECATED
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index c3f871c..a799610 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -371,6 +371,7 @@ const char *av_packet_side_data_name(enum 
AVPacketSideDataType type)
 case AV_PKT_DATA_METADATA_UPDATE:return "Metadata Update";
 case AV_PKT_DATA_MPEGTS_STREAM_ID:   return "MPEGTS Stream ID";
 case AV_PKT_DATA_MASTERING_DISPLAY_METADATA: return "Mastering display 
metadata";
+case AV_PKT_DATA_SPHERICAL:  return "Spherical mapping";
 }
 return NULL;
 }
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index d6dca18..89a12c6 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -762,6 +762,7 @@ int ff_init_buffer_info(AVCodecContext *avctx, AVFrame 
*frame)
 } sd[] = {
 { AV_PKT_DATA_REPLAYGAIN ,AV_FRAME_DATA_REPLAYGAIN },
 { AV_PKT_DATA_DISPLAYMATRIX,  AV_FRAME_DATA_DISPLAYMATRIX 
},
+{ AV_PKT_DATA_SPHERICAL,  AV_FRAME_DATA_SPHERICAL },
 { AV_PKT_DATA_STEREO3D,   AV_FRAME_DATA_STEREO3D },
 { AV_PKT_DATA_AUDIO_SERVICE_TYPE, 
AV_FRAME_DATA_AUDIO_SERVICE_TYPE },
 { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, 
AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
diff --git a/libavformat/dump.c b/libavformat/dump.c
index cd14625..3931a2f 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -31,6 +31,7 @@
 #include "libavutil/opt.h"
 #include "libavutil/avstring.h"
 #include "libavutil/replaygain.h"
+#include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
 
 #include "avformat.h"
@@ -342,6 +343,34 @@ static void dump_mastering_display_metadata(void *ctx, 
AVPacketSideData* sd) {
av_q2d(metadata->min_luminance), av_q2d(metadata->max_luminance));
 }
 
+static void dump_spherical(void *ctx, AVPacketSideData *sd)
+{
+AVSphericalMapping *spherical = (AVSphericalMapping *)sd->data;
+
+if (sd->size < sizeof(*spherical)) {
+av_log(ctx, AV_LOG_INFO, &quo

Re: [FFmpeg-devel] [PATCH 3/3] fate: Add h264 and hevc extradata reload tests

[Vittorio Giovara]

On Wed, Nov 9, 2016 at 4:11 AM, Michael Niedermayer
 wrote:
> On Tue, Nov 08, 2016 at 10:39:41PM -0500, Vittorio Giovara wrote:
>> On Tue, Nov 8, 2016 at 7:15 PM, Michael Niedermayer
>>  wrote:
>> > On Tue, Nov 08, 2016 at 05:03:28PM -0500, Vittorio Giovara wrote:
>> >> ---
>> >> Samples available at
>> >> - 
>> >> https://www.dropbox.com/s/gkswnubzy6e6rgg/h264-extradata-reload-multi-stsd.mov?dl=0
>> >> - 
>> >> https://www.dropbox.com/s/djfvoqgu2z9zrqu/hevc-extradata-reload-multi-stsd.mov?dl=0
>> >> They need to be renamed without the codec tag and placed in the 
>> >> appropriate
>> >> directory in fate.
>> >> Please CC.
>> >> Vittorio
>> >>
>> >>  tests/fate/h264.mak  |  5 +
>> >>  tests/fate/hevc.mak  |  5 +
>> >>  tests/ref/fate/h264-extradata-reload | 13 +
>> >>  tests/ref/fate/hevc-extradata-reload | 13 +
>> >>  4 files changed, 36 insertions(+)
>> >>  create mode 100644 tests/ref/fate/h264-extradata-reload
>> >>  create mode 100644 tests/ref/fate/hevc-extradata-reload
>> >
>> > doesnt seem to pass here
>> >
>> > --- ./tests/ref/fate/hevc-extradata-reload  2016-11-08 
>> > 23:33:06.888177493 +0100
>> > +++ tests/data/fate/hevc-extradata-reload   2016-11-09 
>> > 01:09:12.080298952 +0100
>> > @@ -9,5 +9,5 @@
>> >  #stream#, dts,pts, duration, size, hash
>> >  0,  0,  0,1,24576, 
>> > 0d01217c5d1ec6799643fc7d75ba2337
>> >  0,  1,  1,1,24576, 
>> > f73d9cca9b4c1765d0ead242c3f0c339
>> > -0,  2,  2,1,24576, 
>> > 39a8714d763c623ae7f6faae34e107d1
>> > -0,  3,  3,1,24576, 
>> > 5db2600aa268b4fd28b64ab28a096f32
>> > +0,  2,  2,1,24576, 
>> > aea8b931d694e38ffa54ea4c88e04491
>> > +0,  3,  3,1,24576, 
>> > 9d8f6a78c1bae37eabcab29295fd02a8
>>
>> Oh yeah, I ran the hashes with asm and optimizations disabled, if I do
>> enable them I get the new hashes too.
>> I tried to output the raw images but I can't see any difference, so
>> suspect there could be a lingering issue somewhere in the asm. Would
>> you be able to look into it?
>
> i would but my todo list is too long and growing, maybe someone else
> can help ?

I see, maybe the HEVC part could be stripped out and only the h264
test could be added?
It's good to test multiple stsd for h264 only anyway.

Off topic, what's the status of "mov: Evaluate the movie display
matrix"? I haven't received any more comments.
Please CC.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] fate: Add h264 and hevc extradata reload tests

[Vittorio Giovara]

On Tue, Nov 8, 2016 at 7:15 PM, Michael Niedermayer
 wrote:
> On Tue, Nov 08, 2016 at 05:03:28PM -0500, Vittorio Giovara wrote:
>> ---
>> Samples available at
>> - 
>> https://www.dropbox.com/s/gkswnubzy6e6rgg/h264-extradata-reload-multi-stsd.mov?dl=0
>> - 
>> https://www.dropbox.com/s/djfvoqgu2z9zrqu/hevc-extradata-reload-multi-stsd.mov?dl=0
>> They need to be renamed without the codec tag and placed in the appropriate
>> directory in fate.
>> Please CC.
>> Vittorio
>>
>>  tests/fate/h264.mak  |  5 +
>>  tests/fate/hevc.mak  |  5 +
>>  tests/ref/fate/h264-extradata-reload | 13 +
>>  tests/ref/fate/hevc-extradata-reload | 13 +
>>  4 files changed, 36 insertions(+)
>>  create mode 100644 tests/ref/fate/h264-extradata-reload
>>  create mode 100644 tests/ref/fate/hevc-extradata-reload
>
> doesnt seem to pass here
>
> --- ./tests/ref/fate/hevc-extradata-reload  2016-11-08 23:33:06.888177493 
> +0100
> +++ tests/data/fate/hevc-extradata-reload   2016-11-09 01:09:12.080298952 
> +0100
> @@ -9,5 +9,5 @@
>  #stream#, dts,pts, duration, size, hash
>  0,  0,  0,1,24576, 
> 0d01217c5d1ec6799643fc7d75ba2337
>  0,  1,  1,1,24576, 
> f73d9cca9b4c1765d0ead242c3f0c339
> -0,  2,  2,1,24576, 
> 39a8714d763c623ae7f6faae34e107d1
> -0,  3,  3,1,24576, 
> 5db2600aa268b4fd28b64ab28a096f32
> +0,  2,  2,1,24576, 
> aea8b931d694e38ffa54ea4c88e04491
> +0,  3,  3,1,24576, 
> 9d8f6a78c1bae37eabcab29295fd02a8

Oh yeah, I ran the hashes with asm and optimizations disabled, if I do
enable them I get the new hashes too.
I tried to output the raw images but I can't see any difference, so
suspect there could be a lingering issue somewhere in the asm. Would
you be able to look into it?
Thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/3] hevc: Support extradata changes

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
Applied review.
Please CC.
Vittorio

 libavcodec/hevc.c | 10 ++
 libavformat/mov.c |  4 
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index 02fd606..4417f79 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -3049,6 +3049,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, void 
*data, int *got_output,
  AVPacket *avpkt)
 {
 int ret;
+int new_extradata_size;
+uint8_t *new_extradata;
 HEVCContext *s = avctx->priv_data;
 
 if (!avpkt->size) {
@@ -3060,6 +3062,14 @@ static int hevc_decode_frame(AVCodecContext *avctx, void 
*data, int *got_output,
 return 0;
 }
 
+new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
+&new_extradata_size);
+if (new_extradata && new_extradata_size > 0) {
+ret = hevc_decode_extradata(s, new_extradata, new_extradata_size);
+if (ret < 0)
+return ret;
+}
+
 s->ref = NULL;
 ret= decode_nal_units(s, avpkt->data, avpkt->size);
 if (ret < 0)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2fc09b1..a2a688b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2231,10 +2231,6 @@ static int mov_skip_multiple_stsd(MOVContext *c, 
AVIOContext *pb,
 avio_skip(pb, size);
 return 1;
 }
-if ( codec_tag == AV_RL32("hvc1") ||
- codec_tag == AV_RL32("hev1")
-)
-av_log(c->fc, AV_LOG_WARNING, "Concatenated H.264 or H.265 might not 
play correctly.\n");
 
 return 0;
 }
-- 
2.10.0

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

[FFmpeg-devel] [PATCH 1/3] hevc: Allow parsing external extradata buffers

[Vittorio Giovara]

---
As mentioned in the discussion.
Please CC.
Vittorio

 libavcodec/hevc.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index 29e0d49..02fd606 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -2973,17 +2973,15 @@ static int verify_md5(HEVCContext *s, AVFrame *frame)
 return 0;
 }
 
-static int hevc_decode_extradata(HEVCContext *s)
+static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length)
 {
 AVCodecContext *avctx = s->avctx;
 GetByteContext gb;
 int ret, i;
 
-bytestream2_init(&gb, avctx->extradata, avctx->extradata_size);
+bytestream2_init(&gb, buf, length);
 
-if (avctx->extradata_size > 3 &&
-(avctx->extradata[0] || avctx->extradata[1] ||
- avctx->extradata[2] > 1)) {
+if (avctx->extradata_size > 3 && (buf[0] || buf[1] || buf[2] > 1)) {
 /* It seems the extradata is encoded as hvcC format.
  * Temporarily, we support configurationVersion==0 until 14496-15 3rd
  * is finalized. When finalized, configurationVersion will be 1 and we
@@ -3030,7 +3028,7 @@ static int hevc_decode_extradata(HEVCContext *s)
 s->nal_length_size = nal_len_size;
 } else {
 s->is_nalff = 0;
-ret = decode_nal_units(s, avctx->extradata, avctx->extradata_size);
+ret = decode_nal_units(s, buf, length);
 if (ret < 0)
 return ret;
 }
@@ -3338,7 +3336,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx)
 s->threads_number = 1;
 
 if (avctx->extradata_size > 0 && avctx->extradata) {
-ret = hevc_decode_extradata(s);
+ret = hevc_decode_extradata(s, avctx->extradata, 
avctx->extradata_size);
 if (ret < 0) {
 hevc_decode_free(avctx);
 return ret;
-- 
2.10.0

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

[FFmpeg-devel] [PATCH 3/3] fate: Add h264 and hevc extradata reload tests

[Vittorio Giovara]

---
Samples available at
- 
https://www.dropbox.com/s/gkswnubzy6e6rgg/h264-extradata-reload-multi-stsd.mov?dl=0
- 
https://www.dropbox.com/s/djfvoqgu2z9zrqu/hevc-extradata-reload-multi-stsd.mov?dl=0
They need to be renamed without the codec tag and placed in the appropriate
directory in fate.
Please CC.
Vittorio

 tests/fate/h264.mak  |  5 +
 tests/fate/hevc.mak  |  5 +
 tests/ref/fate/h264-extradata-reload | 13 +
 tests/ref/fate/hevc-extradata-reload | 13 +
 4 files changed, 36 insertions(+)
 create mode 100644 tests/ref/fate/h264-extradata-reload
 create mode 100644 tests/ref/fate/hevc-extradata-reload

diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak
index b4d7f7a..f8ef3f4 100644
--- a/tests/fate/h264.mak
+++ b/tests/fate/h264.mak
@@ -196,6 +196,10 @@ FATE_H264  := $(FATE_H264:%=fate-h264-conformance-%)   
 \
 
 FATE_H264-$(call DEMDEC, H264, H264) += $(FATE_H264)
 FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-crop-to-container
+
+# this sample has two stsd entries and needs to reload extradata
+FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-extradata-reload
+
 FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-interlace-crop
 
 # this sample has invalid reference list modification, but decodes fine
@@ -408,6 +412,7 @@ fate-h264-conformance-sva_nl2_e:  CMD = 
framecrc -vsync drop -i
 fate-h264-bsf-mp4toannexb:CMD = md5 -i 
$(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vcodec copy -f h264
 
 fate-h264-crop-to-container:  CMD = framemd5 -i 
$(TARGET_SAMPLES)/h264/crop-to-container-dims-canon.mov
+fate-h264-extradata-reload:   CMD = framemd5 -i 
$(TARGET_SAMPLES)/h264/extradata-reload-multi-stsd.mov
 fate-h264-extreme-plane-pred: CMD = framemd5 -i 
$(TARGET_SAMPLES)/h264/extreme-plane-pred.h264
 fate-h264-interlace-crop: CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vframes 3
 fate-h264-lossless:   CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/lossless.h264
diff --git a/tests/fate/hevc.mak b/tests/fate/hevc.mak
index bd09ab3..9150986 100644
--- a/tests/fate/hevc.mak
+++ b/tests/fate/hevc.mak
@@ -238,6 +238,11 @@ fate-hevc-bsf-mp4toannexb: REF = 
1873662a3af1848c37e4eb25722c8df9
 
 FATE_HEVC-$(call DEMDEC, HEVC, HEVC) += $(FATE_HEVC)
 
+# this sample has two stsd entries and needs to reload extradata
+FATE_HEVC-$(call DEMDEC, MOV, HEVC) += fate-hevc-extradata-reload
+
+fate-hevc-extradata-reload: CMD = framemd5 -i 
$(TARGET_SAMPLES)/hevc/extradata-reload-multi-stsd.mov
+
 FATE_SAMPLES_AVCONV += $(FATE_HEVC-yes)
 
 fate-hevc: $(FATE_HEVC-yes)
diff --git a/tests/ref/fate/h264-extradata-reload 
b/tests/ref/fate/h264-extradata-reload
new file mode 100644
index 000..c70f805
--- /dev/null
+++ b/tests/ref/fate/h264-extradata-reload
@@ -0,0 +1,13 @@
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 256x128
+#sar 0: 1/1
+#stream#, dts,pts, duration, size, hash
+0,  0,  0,1,49152, ae09c88e87e3ea0aa8ad267ee91222e5
+0,  1,  1,1,49152, 7ccd8321a6e23ae1f82bd323c8376524
+0,  2,  2,1,49152, 8ed93ab585ff9848801cc28b75b5e12d
+0,  3,  3,1,49152, 0049913870ddb62ffc535282018766f4
diff --git a/tests/ref/fate/hevc-extradata-reload 
b/tests/ref/fate/hevc-extradata-reload
new file mode 100644
index 000..b08754e
--- /dev/null
+++ b/tests/ref/fate/hevc-extradata-reload
@@ -0,0 +1,13 @@
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 128x128
+#sar 0: 1/1
+#stream#, dts,pts, duration, size, hash
+0,  0,  0,1,24576, 0d01217c5d1ec6799643fc7d75ba2337
+0,  1,  1,1,24576, f73d9cca9b4c1765d0ead242c3f0c339
+0,  2,  2,1,24576, 39a8714d763c623ae7f6faae34e107d1
+0,  3,  3,1,24576, 5db2600aa268b4fd28b64ab28a096f32
-- 
2.10.0

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

[FFmpeg-devel] [PATCH] mov: Read multiple stsd from DV

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
Sorry, I can't share the sample or add a fate for this.
Please CC.
Vittorio

 libavformat/mov.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index e283034..a2a688b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2219,6 +2219,8 @@ static int mov_skip_multiple_stsd(MOVContext *c, 
AVIOContext *pb,
  (codec_tag != format &&
   // prores is allowed to have differing data format and codec tag
   codec_tag != AV_RL32("apcn") && codec_tag != AV_RL32("apch") &&
+  // so is dv (sigh)
+  codec_tag != AV_RL32("dvpp") && codec_tag != AV_RL32("dvcp") &&
   (c->fc->video_codec_id ? video_codec_id != c->fc->video_codec_id
  : codec_tag != MKTAG('j','p','e','g' {
 /* Multiple fourcc, we skip JPEG. This is not correct, we should
-- 
2.10.0

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

Re: [FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Vittorio Giovara]

On Mon, Nov 7, 2016 at 6:44 PM, Michael Niedermayer
 wrote:
> the decoder should not change extradata, yes directly reading would
> be best probably

ok I'll amend.

>>
>> > also can you add a fate test ?
>>
>> I have a sample I can share but it's incredibly big for a fate test (85mb).
>
> :(
>
> cant it be cut and glued so its smaller ?

Someone tipped me of a way, I'll try.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Vittorio Giovara]

On Sat, Nov 5, 2016 at 9:21 AM, Michael Niedermayer
 wrote:
> On Wed, Nov 02, 2016 at 11:48:58AM -0400, Vittorio Giovara wrote:
>> Signed-off-by: Vittorio Giovara 
>> ---
>> Please CC.
>> Vittorio
>>
>>  libavcodec/hevc.c | 18 ++
>>  libavformat/mov.c |  4 
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
>> index 29e0d49..b50120e 100644
>> --- a/libavcodec/hevc.c
>> +++ b/libavcodec/hevc.c
>> @@ -3051,6 +3051,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, 
>> void *data, int *got_output,
>>   AVPacket *avpkt)
>>  {
>>  int ret;
>> +int new_extradata_size;
>> +uint8_t *new_extradata;
>>  HEVCContext *s = avctx->priv_data;
>>
>>  if (!avpkt->size) {
>> @@ -3062,6 +3064,22 @@ static int hevc_decode_frame(AVCodecContext *avctx, 
>> void *data, int *got_output,
>>  return 0;
>>  }
>>
>> +new_extradata_size = 0;
>> +new_extradata = av_packet_get_side_data(avpkt, 
>> AV_PKT_DATA_NEW_EXTRADATA,
>> +&new_extradata_size);
>> +if (new_extradata_size > 0 && new_extradata) {
>
> new_extradata should be checked first, that should make
> new_extradata_size = 0; unneeded

ok

>> +if (new_extradata_size > avctx->extradata_size) {
>
>> +avctx->extradata = av_realloc(avctx->extradata, 
>> new_extradata_size);
>
> This leaks on reallocation failure overwriting the allocated pointer

yeah, also, extradata is av_malloc'd, it is not possible call any
*realloc* functions at all.

I thought of av_free + av_malloc but I'm afraid a free there will
interfere with frame multi threading, like it did for h264. So maybe
it's better to modify hevc_decode_extradata() to support reading
extradata from input buffers rather than from avctx (like h264 does).
Thoughts?

> also can you add a fate test ?

I have a sample I can share but it's incredibly big for a fate test (85mb).
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] hevc: Move hevc_decode_extradata before frame decoding

[Vittorio Giovara]

On Thu, Nov 3, 2016 at 2:06 PM, Michael Niedermayer
 wrote:
> On Wed, Nov 02, 2016 at 11:48:57AM -0400, Vittorio Giovara wrote:
>> Avoids a forward-declaration in the following commit.
>>
>> Signed-off-by: Vittorio Giovara 
>> ---
>> Please CC.
>> Vittorio
>
> applied
>
> thx

Thanks, but what about 2/2? These two commits were closely tied so
maybe they should have been queued together.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCHv4] mov: Evaluate the movie display matrix

[Vittorio Giovara]

This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet. So store it in main context, and apply
it when appropriate, that is after parsing the tkhd one.

Signed-off-by: Vittorio Giovara 
---
Updated according everybody's review. Changed the loop to always perform
multiplication, so that it's simpler to apply and to test. Changed computation
to fixed point instead of conversion to double.

Added a single test to cover this usecase, 
https://www.dropbox.com/s/qfio4bjhkpz3p4o/displaymatrix.mov?dl=0
Please CC.
Vittorio

 libavformat/isom.h   |  1 +
 libavformat/mov.c| 48 +---
 tests/fate/mov.mak   |  5 -
 tests/ref/fate/mov-displaymatrix | 10 +
 4 files changed, 50 insertions(+), 14 deletions(-)
 create mode 100644 tests/ref/fate/mov-displaymatrix

diff --git a/libavformat/isom.h b/libavformat/isom.h
index d684502..02bfedd 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -240,6 +240,7 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 35782fe..beedaef 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1231,6 +1231,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1258,7 +1259,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3815,12 +3821,22 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 return 0;
 }
 
+// return 1 when matrix is identity, 0 otherwise
+#define IS_MATRIX_IDENT(matrix)\
+( (matrix)[0][0] == (1 << 16) &&   \
+  (matrix)[1][1] == (1 << 16) &&   \
+  (matrix)[2][2] == (1 << 30) &&   \
+ !(matrix)[0][1] && !(matrix)[0][2] || \
+ !(matrix)[1][0] && !(matrix)[1][2] || \
+ !(matrix)[2][0] && !(matrix)[2][1])
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
+int res_display_matrix[3][3] = { { 0 } };
 AVStream *st;
 MOVStreamContext *sc;
 int version;
@@ -3870,15 +3886,20 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 sc->width = width >> 16;
 sc->height = height >> 16;
 
-// save the matrix and add rotate metadata when it is not the default
-// identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+// apply the moov display matrix (after the tkhd one)
+for (i = 0; i < 3; i++) {
+const int sh[3] = { 16, 16, 30 };
+for (j = 0; j < 3; j++) {
+for (e = 0; e < 3; e++) {
+res_display_matrix[i][j] +=
+((int64_t) display_matrix[i][e] *
+ c->movie_display_matrix[e][j]) >> sh[e];
+}
+}
+}
+
+// save the matrix when it is not the default identity
+if (!IS_MATRIX_IDENT(res_display_matrix)) {
 double rotate;
 
 av_freep(&sc->display_matrix);
@@ -3888,7 +3909,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 for (i = 0; i < 3; i++)
 for (j = 0; j < 3; j++)
-sc->display_matrix[i * 3 + j] = display_matrix[i][j];
+sc->display_matrix[i * 3 + j] = res_display_matrix[i][j];
 
 rotate = av_display_rotation_get(sc->display_matrix);
 if (!isnan(rotate)) {
@@ -3907,7 +3928,8 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 double d

[FFmpeg-devel] [PATCH 2/2] hevc: Support extradata changes

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
Please CC.
Vittorio

 libavcodec/hevc.c | 18 ++
 libavformat/mov.c |  4 
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index 29e0d49..b50120e 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -3051,6 +3051,8 @@ static int hevc_decode_frame(AVCodecContext *avctx, void 
*data, int *got_output,
  AVPacket *avpkt)
 {
 int ret;
+int new_extradata_size;
+uint8_t *new_extradata;
 HEVCContext *s = avctx->priv_data;
 
 if (!avpkt->size) {
@@ -3062,6 +3064,22 @@ static int hevc_decode_frame(AVCodecContext *avctx, void 
*data, int *got_output,
 return 0;
 }
 
+new_extradata_size = 0;
+new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
+&new_extradata_size);
+if (new_extradata_size > 0 && new_extradata) {
+if (new_extradata_size > avctx->extradata_size) {
+avctx->extradata = av_realloc(avctx->extradata, 
new_extradata_size);
+if (!avctx->extradata)
+return AVERROR(ENOMEM);
+}
+avctx->extradata_size = new_extradata_size;
+memcpy(avctx->extradata, new_extradata, new_extradata_size);
+ret = hevc_decode_extradata(s);
+if (ret < 0)
+return ret;
+}
+
 s->ref = NULL;
 ret= decode_nal_units(s, avpkt->data, avpkt->size);
 if (ret < 0)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 4222088..24c75ab 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2212,10 +2212,6 @@ static int mov_skip_multiple_stsd(MOVContext *c, 
AVIOContext *pb,
 avio_skip(pb, size);
 return 1;
 }
-if ( codec_tag == AV_RL32("hvc1") ||
- codec_tag == AV_RL32("hev1")
-)
-av_log(c->fc, AV_LOG_WARNING, "Concatenated H.264 or H.265 might not 
play correctly.\n");
 
 return 0;
 }
-- 
2.10.0

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

[FFmpeg-devel] [PATCH 1/2] hevc: Move hevc_decode_extradata before frame decoding

[Vittorio Giovara]

Avoids a forward-declaration in the following commit.

Signed-off-by: Vittorio Giovara 
---
Please CC.
Vittorio

libavcodec/hevc.c | 148 +++---
 1 file changed, 74 insertions(+), 74 deletions(-)

diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index 764e093..29e0d49 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -2973,6 +2973,80 @@ static int verify_md5(HEVCContext *s, AVFrame *frame)
 return 0;
 }
 
+static int hevc_decode_extradata(HEVCContext *s)
+{
+AVCodecContext *avctx = s->avctx;
+GetByteContext gb;
+int ret, i;
+
+bytestream2_init(&gb, avctx->extradata, avctx->extradata_size);
+
+if (avctx->extradata_size > 3 &&
+(avctx->extradata[0] || avctx->extradata[1] ||
+ avctx->extradata[2] > 1)) {
+/* It seems the extradata is encoded as hvcC format.
+ * Temporarily, we support configurationVersion==0 until 14496-15 3rd
+ * is finalized. When finalized, configurationVersion will be 1 and we
+ * can recognize hvcC by checking if avctx->extradata[0]==1 or not. */
+int i, j, num_arrays, nal_len_size;
+
+s->is_nalff = 1;
+
+bytestream2_skip(&gb, 21);
+nal_len_size = (bytestream2_get_byte(&gb) & 3) + 1;
+num_arrays   = bytestream2_get_byte(&gb);
+
+/* nal units in the hvcC always have length coded with 2 bytes,
+ * so put a fake nal_length_size = 2 while parsing them */
+s->nal_length_size = 2;
+
+/* Decode nal units from hvcC. */
+for (i = 0; i < num_arrays; i++) {
+int type = bytestream2_get_byte(&gb) & 0x3f;
+int cnt  = bytestream2_get_be16(&gb);
+
+for (j = 0; j < cnt; j++) {
+// +2 for the nal size field
+int nalsize = bytestream2_peek_be16(&gb) + 2;
+if (bytestream2_get_bytes_left(&gb) < nalsize) {
+av_log(s->avctx, AV_LOG_ERROR,
+   "Invalid NAL unit size in extradata.\n");
+return AVERROR_INVALIDDATA;
+}
+
+ret = decode_nal_units(s, gb.buffer, nalsize);
+if (ret < 0) {
+av_log(avctx, AV_LOG_ERROR,
+   "Decoding nal unit %d %d from hvcC failed\n",
+   type, i);
+return ret;
+}
+bytestream2_skip(&gb, nalsize);
+}
+}
+
+/* Now store right nal length size, that will be used to parse
+ * all other nals */
+s->nal_length_size = nal_len_size;
+} else {
+s->is_nalff = 0;
+ret = decode_nal_units(s, avctx->extradata, avctx->extradata_size);
+if (ret < 0)
+return ret;
+}
+
+/* export stream parameters from the first SPS */
+for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) {
+if (s->ps.sps_list[i]) {
+const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data;
+export_stream_params(s->avctx, &s->ps, sps);
+break;
+}
+}
+
+return 0;
+}
+
 static int hevc_decode_frame(AVCodecContext *avctx, void *data, int 
*got_output,
  AVPacket *avpkt)
 {
@@ -3243,80 +3317,6 @@ static int hevc_update_thread_context(AVCodecContext 
*dst,
 return 0;
 }
 
-static int hevc_decode_extradata(HEVCContext *s)
-{
-AVCodecContext *avctx = s->avctx;
-GetByteContext gb;
-int ret, i;
-
-bytestream2_init(&gb, avctx->extradata, avctx->extradata_size);
-
-if (avctx->extradata_size > 3 &&
-(avctx->extradata[0] || avctx->extradata[1] ||
- avctx->extradata[2] > 1)) {
-/* It seems the extradata is encoded as hvcC format.
- * Temporarily, we support configurationVersion==0 until 14496-15 3rd
- * is finalized. When finalized, configurationVersion will be 1 and we
- * can recognize hvcC by checking if avctx->extradata[0]==1 or not. */
-int i, j, num_arrays, nal_len_size;
-
-s->is_nalff = 1;
-
-bytestream2_skip(&gb, 21);
-nal_len_size = (bytestream2_get_byte(&gb) & 3) + 1;
-num_arrays   = bytestream2_get_byte(&gb);
-
-/* nal units in the hvcC always have length coded with 2 bytes,
- * so put a fake nal_length_size = 2 while parsing them */
-s->nal_length_size = 2;
-
-/* Decode nal units from hvcC. */
-for (i = 0; i < num_arrays; i++) {
-int type = bytestream2_get_byte(&gb) & 0x3f;
-int cnt  = bytestream2_get_be16(&gb);
-
-for (j = 0; j < cnt; j++) {
-// +2 for the nal size field
- 

[FFmpeg-devel] [PATCH] lavc: Add hevc main10 profile to ffmpeg cli

[Vittorio Giovara]

---
Please CC.
Vittorio

 libavcodec/options_table.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index 995906b..4323ae9 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -394,6 +394,7 @@ static const AVOption avcodec_options[] = {
 {"mpeg4_core", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_CORE }, 
INT_MIN, INT_MAX, V|E, "profile"},
 {"mpeg4_main", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_MAIN }, 
INT_MIN, INT_MAX, V|E, "profile"},
 {"mpeg4_asp",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 
FF_PROFILE_MPEG4_ADVANCED_SIMPLE }, INT_MIN, INT_MAX, V|E, "profile"},
+{"main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_HEVC_MAIN_10 }, 
INT_MIN, INT_MAX, V|E, "profile"},
 {"level", NULL, OFFSET(level), AV_OPT_TYPE_INT, {.i64 = FF_LEVEL_UNKNOWN }, 
INT_MIN, INT_MAX, V|A|E, "level"},
 {"unknown", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_LEVEL_UNKNOWN }, INT_MIN, 
INT_MAX, V|A|E, "level"},
 {"lowres", "decode at 1= 1/2, 2=1/4, 3=1/8 resolutions", OFFSET(lowres), 
AV_OPT_TYPE_INT, {.i64 = 0 }, 0, INT_MAX, V|A|D},
-- 
2.10.0

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

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Add support for smpte 431/432 (dci/display p3) primaries

[Vittorio Giovara]

On Mon, Oct 31, 2016 at 4:08 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Mon, Oct 31, 2016 at 3:31 PM, Vittorio Giovara
>  wrote:
>>
>> On Mon, Oct 31, 2016 at 2:53 PM, Ronald S. Bultje 
>> wrote:
>> > Hi,
>> >
>> > On Mon, Oct 31, 2016 at 11:13 AM, Vittorio Giovara
>> >  wrote:
>> >>
>> >> On Mon, Oct 31, 2016 at 7:43 AM, Ronald S. Bultje 
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > On Mon, Oct 31, 2016 at 5:50 AM, Kevin Wheatley
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Sun, Oct 30, 2016 at 1:18 PM, Ronald S. Bultje
>> >> >> 
>> >> >> wrote:
>> >> >> > Hmm... So, the wikipedia page https://en.wikipedia.org/wiki/DCI-P3
>> >> >> > refers
>> >> >> > to the two whitepoints here as DCI-P3 D65 and DCI-P3 Theater.
>> >> >> > Calling
>> >> >> > one
>> >> >> > D65 and the other DCI seems confusing in that light (assuming the
>> >> >> > wikipedia
>> >> >> > page is correct). I'd call it THEATER or DCI_P3_THEATER, to
>> >> >> > distinguish
>> >> >> > it
>> >> >> > from DCI-P3 D65. Is that OK?
>> >> >>
>> >> >> In the industry people just call it the DCI P3 white point (or
>> >> >> 'Urgh')
>> >> >> it is not limited to theater usage, you might consider it the
>> >> >> calibration white point for the reference projector, so
>> >> >> WP_DCI_P3_REFERENCE might be better, but that is a little long.
>> >> >>
>> >> >> I'd go for something like WP_DCI_P3 it is not really ambiguous.
>> >> >
>> >> >
>> >> > Hm... OK with me (though not ideal, but what do I know). Vittorio, OK
>> >> > also?
>> >> > I can modify patch so you don't have to resend.
>> >>
>> >> I find it a little long and not less confusing than my initial WP_DCI,
>> >> among all the alternatives I liked the THEATER one the most. If that's
>> >> a no-go, how about we could settle for WP_PROJ maybe?
>> >
>> >
>> > Wait, wait. Length is an issue? Really?
>> >
>> > The only reason the other names are short is because the names of the
>> > whitepoints are short. D65 is really just called that: D65. Likewise for
>> > D50. This name (whatever it is :D) is simply longer.
>>
>> It's not a matter of length but a matter of descriptiveness: right now
>> there is only one single different whitepoint defined by DCI, so IMO
>> it makes sense to call it simply WP_DCI. In case DCI adds new values,
>> naming can be modified later. The other whitepoints could also have
>> longer, more descriptive names too, like WP_ILLUMINANT_C, but at the
>> same time the WP_C shorthand is convenient and immediate (and IMO
>> better suited as variable name).
>
>
> That's actually a good point. I'm not sure if C is better than
> ILLUMINANT_C... WDYT? I guess you're sticking to the "shorter is better"? :)

In this case, yes, shorter is better, in my opinion.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Add support for smpte 431/432 (dci/display p3) primaries

[Vittorio Giovara]

On Mon, Oct 31, 2016 at 2:53 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Mon, Oct 31, 2016 at 11:13 AM, Vittorio Giovara
>  wrote:
>>
>> On Mon, Oct 31, 2016 at 7:43 AM, Ronald S. Bultje 
>> wrote:
>> > Hi,
>> >
>> > On Mon, Oct 31, 2016 at 5:50 AM, Kevin Wheatley
>> > 
>> > wrote:
>> >>
>> >> On Sun, Oct 30, 2016 at 1:18 PM, Ronald S. Bultje 
>> >> wrote:
>> >> > Hmm... So, the wikipedia page https://en.wikipedia.org/wiki/DCI-P3
>> >> > refers
>> >> > to the two whitepoints here as DCI-P3 D65 and DCI-P3 Theater. Calling
>> >> > one
>> >> > D65 and the other DCI seems confusing in that light (assuming the
>> >> > wikipedia
>> >> > page is correct). I'd call it THEATER or DCI_P3_THEATER, to
>> >> > distinguish
>> >> > it
>> >> > from DCI-P3 D65. Is that OK?
>> >>
>> >> In the industry people just call it the DCI P3 white point (or 'Urgh')
>> >> it is not limited to theater usage, you might consider it the
>> >> calibration white point for the reference projector, so
>> >> WP_DCI_P3_REFERENCE might be better, but that is a little long.
>> >>
>> >> I'd go for something like WP_DCI_P3 it is not really ambiguous.
>> >
>> >
>> > Hm... OK with me (though not ideal, but what do I know). Vittorio, OK
>> > also?
>> > I can modify patch so you don't have to resend.
>>
>> I find it a little long and not less confusing than my initial WP_DCI,
>> among all the alternatives I liked the THEATER one the most. If that's
>> a no-go, how about we could settle for WP_PROJ maybe?
>
>
> Wait, wait. Length is an issue? Really?
>
> The only reason the other names are short is because the names of the
> whitepoints are short. D65 is really just called that: D65. Likewise for
> D50. This name (whatever it is :D) is simply longer.

It's not a matter of length but a matter of descriptiveness: right now
there is only one single different whitepoint defined by DCI, so IMO
it makes sense to call it simply WP_DCI. In case DCI adds new values,
naming can be modified later. The other whitepoints could also have
longer, more descriptive names too, like WP_ILLUMINANT_C, but at the
same time the WP_C shorthand is convenient and immediate (and IMO
better suited as variable name).

At any rate, pick the one you prefer ;)
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] vf_colorspace: Add support for film primaries

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
This is the last easy picking, the remaining trc/prm require more convoluted
changes to the filter that will need more discussion.
Please CC.
Vittorio

 libavfilter/vf_colorspace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 4265aa1..997c594 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -278,6 +278,7 @@ static const struct ColorPrimaries 
color_primaries[AVCOL_PRI_NB] = {
 [AVCOL_PRI_BT470BG]   = { WP_D65, 0.640, 0.330, 0.290, 0.600, 0.150, 
0.060,},
 [AVCOL_PRI_SMPTE170M] = { WP_D65, 0.630, 0.340, 0.310, 0.595, 0.155, 0.070 
},
 [AVCOL_PRI_SMPTE240M] = { WP_D65, 0.630, 0.340, 0.310, 0.595, 0.155, 0.070 
},
+[AVCOL_PRI_FILM]  = { WP_C,   0.681, 0.319, 0.243, 0.692, 0.145, 0.049 
},
 [AVCOL_PRI_SMPTE431]  = { WP_DCI, 0.680, 0.320, 0.265, 0.690, 0.150, 0.060 
},
 [AVCOL_PRI_SMPTE432]  = { WP_D65, 0.680, 0.320, 0.265, 0.690, 0.150, 0.060 
},
 [AVCOL_PRI_BT2020]= { WP_D65, 0.708, 0.292, 0.170, 0.797, 0.131, 0.046 
},
@@ -1084,6 +1085,7 @@ static const AVOption colorspace_options[] = {
 ENUM("bt470bg",  AVCOL_PRI_BT470BG,"prm"),
 ENUM("smpte170m",AVCOL_PRI_SMPTE170M,  "prm"),
 ENUM("smpte240m",AVCOL_PRI_SMPTE240M,  "prm"),
+ENUM("film", AVCOL_PRI_FILM,   "prm"),
 ENUM("smpte431", AVCOL_PRI_SMPTE431,   "prm"),
 ENUM("smpte432", AVCOL_PRI_SMPTE432,   "prm"),
 ENUM("bt2020",   AVCOL_PRI_BT2020, "prm"),
-- 
2.10.0

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

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Add support for smpte 431/432 (dci/display p3) primaries

[Vittorio Giovara]

On Mon, Oct 31, 2016 at 7:43 AM, Ronald S. Bultje  wrote:
> Hi,
>
> On Mon, Oct 31, 2016 at 5:50 AM, Kevin Wheatley 
> wrote:
>>
>> On Sun, Oct 30, 2016 at 1:18 PM, Ronald S. Bultje 
>> wrote:
>> > Hmm... So, the wikipedia page https://en.wikipedia.org/wiki/DCI-P3
>> > refers
>> > to the two whitepoints here as DCI-P3 D65 and DCI-P3 Theater. Calling
>> > one
>> > D65 and the other DCI seems confusing in that light (assuming the
>> > wikipedia
>> > page is correct). I'd call it THEATER or DCI_P3_THEATER, to distinguish
>> > it
>> > from DCI-P3 D65. Is that OK?
>>
>> In the industry people just call it the DCI P3 white point (or 'Urgh')
>> it is not limited to theater usage, you might consider it the
>> calibration white point for the reference projector, so
>> WP_DCI_P3_REFERENCE might be better, but that is a little long.
>>
>> I'd go for something like WP_DCI_P3 it is not really ambiguous.
>
>
> Hm... OK with me (though not ideal, but what do I know). Vittorio, OK also?
> I can modify patch so you don't have to resend.

I find it a little long and not less confusing than my initial WP_DCI,
among all the alternatives I liked the THEATER one the most. If that's
a no-go, how about we could settle for WP_PROJ maybe?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/3] vf_colorspace: Add support for ycgco color space

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
This is a little hackish, not sure how to represent the matrix otherwise.
Please CC.
Vittorio

 libavfilter/vf_colorspace.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index d26f658..7e0bafa 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -175,6 +175,13 @@ typedef struct ColorSpaceContext {
 // FIXME dithering if bitdepth goes down?
 // FIXME bitexact for fate integration?
 
+static const double ycgco_matrix[3][3] =
+{
+{  0.25, 0.5,  0.25 },
+{ -0.25, 0.5, -0.25 },
+{  0.5,  0,   -0.5  },
+};
+
 /*
  * All constants explained in e.g. 
https://linuxtv.org/downloads/v4l-dvb-apis/ch02s06.html
  * The older ones (bt470bg/m) are also explained in their respective ITU docs
@@ -187,6 +194,7 @@ static const struct LumaCoefficients 
luma_coefficients[AVCOL_SPC_NB] = {
 [AVCOL_SPC_SMPTE170M]  = { 0.299,  0.587,  0.114  },
 [AVCOL_SPC_BT709]  = { 0.2126, 0.7152, 0.0722 },
 [AVCOL_SPC_SMPTE240M]  = { 0.212,  0.701,  0.087  },
+[AVCOL_SPC_YCOCG]  = { 0.25,   0.5,0.25   },
 [AVCOL_SPC_BT2020_NCL] = { 0.2627, 0.6780, 0.0593 },
 [AVCOL_SPC_BT2020_CL]  = { 0.2627, 0.6780, 0.0593 },
 };
@@ -209,6 +217,12 @@ static void fill_rgb2yuv_table(const struct 
LumaCoefficients *coeffs,
 {
 double bscale, rscale;
 
+// special ycgco matrix
+if (coeffs->cr == 0.25 && coeffs->cg == 0.5 && coeffs->cb == 0.25) {
+memcpy(rgb2yuv, ycgco_matrix, sizeof(double) * 9);
+return;
+}
+
 rgb2yuv[0][0] = coeffs->cr;
 rgb2yuv[0][1] = coeffs->cg;
 rgb2yuv[0][2] = coeffs->cb;
@@ -1047,6 +1061,7 @@ static const AVOption colorspace_options[] = {
 ENUM("bt470bg", AVCOL_SPC_BT470BG, "csp"),
 ENUM("smpte170m",   AVCOL_SPC_SMPTE170M,   "csp"),
 ENUM("smpte240m",   AVCOL_SPC_SMPTE240M,   "csp"),
+ENUM("ycgco",   AVCOL_SPC_YCGCO,   "csp"),
 ENUM("bt2020ncl",   AVCOL_SPC_BT2020_NCL,  "csp"),
 
 { "range",  "Output color range",
-- 
2.10.0

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

[FFmpeg-devel] [PATCH 3/3] vf_colorspace: Add support for smpte 431/432 (dci/display p3) primaries

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
I couldn't find any reference to the name of the whitepoint used for 431,
so I came up with DCI, since it looks like it is only used there.
Please CC.
Vittorio

 libavfilter/vf_colorspace.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 7e0bafa..4265aa1 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -56,6 +56,7 @@ enum Colorspace {
 enum Whitepoint {
 WP_D65,
 WP_C,
+WP_DCI,
 WP_NB,
 };
 
@@ -268,6 +269,7 @@ static const struct TransferCharacteristics *
 static const struct WhitepointCoefficients whitepoint_coefficients[WP_NB] = {
 [WP_D65] = { 0.3127, 0.3290 },
 [WP_C]   = { 0.3100, 0.3160 },
+[WP_DCI] = { 0.3140, 0.3510 },
 };
 
 static const struct ColorPrimaries color_primaries[AVCOL_PRI_NB] = {
@@ -276,6 +278,8 @@ static const struct ColorPrimaries 
color_primaries[AVCOL_PRI_NB] = {
 [AVCOL_PRI_BT470BG]   = { WP_D65, 0.640, 0.330, 0.290, 0.600, 0.150, 
0.060,},
 [AVCOL_PRI_SMPTE170M] = { WP_D65, 0.630, 0.340, 0.310, 0.595, 0.155, 0.070 
},
 [AVCOL_PRI_SMPTE240M] = { WP_D65, 0.630, 0.340, 0.310, 0.595, 0.155, 0.070 
},
+[AVCOL_PRI_SMPTE431]  = { WP_DCI, 0.680, 0.320, 0.265, 0.690, 0.150, 0.060 
},
+[AVCOL_PRI_SMPTE432]  = { WP_D65, 0.680, 0.320, 0.265, 0.690, 0.150, 0.060 
},
 [AVCOL_PRI_BT2020]= { WP_D65, 0.708, 0.292, 0.170, 0.797, 0.131, 0.046 
},
 };
 
@@ -1080,6 +1084,8 @@ static const AVOption colorspace_options[] = {
 ENUM("bt470bg",  AVCOL_PRI_BT470BG,"prm"),
 ENUM("smpte170m",AVCOL_PRI_SMPTE170M,  "prm"),
 ENUM("smpte240m",AVCOL_PRI_SMPTE240M,  "prm"),
+ENUM("smpte431", AVCOL_PRI_SMPTE431,   "prm"),
+ENUM("smpte432", AVCOL_PRI_SMPTE432,   "prm"),
 ENUM("bt2020",   AVCOL_PRI_BT2020, "prm"),
 
 { "trc","Output transfer characteristics",
-- 
2.10.0

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

[FFmpeg-devel] [PATCH 1/3] vf_colorspace: Add support for iec61966-2.4 (xvYCC) transfer

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
As described in http://www.color.org/chardata/rgb/xvycc.xalter
Please CC.
Vittorio

 libavfilter/vf_colorspace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 930aa95..d26f658 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -232,6 +232,7 @@ static const struct TransferCharacteristics 
transfer_characteristics[AVCOL_TRC_N
 [AVCOL_TRC_SMPTE170M] = { 1.099,  0.018,  0.45, 4.5 },
 [AVCOL_TRC_SMPTE240M] = { 1.1115, 0.0228, 0.45, 4.0 },
 [AVCOL_TRC_IEC61966_2_1] = { 1.055, 0.0031308, 1.0 / 2.4, 12.92 },
+[AVCOL_TRC_IEC61966_2_4] = { 1.099, 0.018, 0.45, 4.5 },
 [AVCOL_TRC_BT2020_10] = { 1.099,  0.018,  0.45, 4.5 },
 [AVCOL_TRC_BT2020_12] = { 1.0993, 0.0181, 0.45, 4.5 },
 };
@@ -1078,6 +1079,8 @@ static const AVOption colorspace_options[] = {
 ENUM("smpte240m",AVCOL_TRC_SMPTE240M,"trc"),
 ENUM("srgb", AVCOL_TRC_IEC61966_2_1, "trc"),
 ENUM("iec61966-2-1", AVCOL_TRC_IEC61966_2_1, "trc"),
+ENUM("xvycc",AVCOL_TRC_IEC61966_2_4, "trc"),
+ENUM("iec61966-2-4", AVCOL_TRC_IEC61966_2_4, "trc"),
 ENUM("bt2020-10",AVCOL_TRC_BT2020_10,"trc"),
 ENUM("bt2020-12",AVCOL_TRC_BT2020_12,"trc"),
 
-- 
2.10.0

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

[FFmpeg-devel] [PATCH] vf_colorspace: Add support for iec61966-2.1 (sRGB) transfer

[Vittorio Giovara]

Signed-off-by: Vittorio Giovara 
---
Special thanks to Andrew Roland for helping me figure out these numbers.
Please keep me in CC.
Vittorio

 libavfilter/vf_colorspace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 5b060f9..16ad760 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -229,6 +229,7 @@ static const struct TransferCharacteristics 
transfer_characteristics[AVCOL_TRC_N
 [AVCOL_TRC_GAMMA28]   = { 1.0,0.0,1.0 / 2.8, 0.0 },
 [AVCOL_TRC_SMPTE170M] = { 1.099,  0.018,  0.45, 4.5 },
 [AVCOL_TRC_SMPTE240M] = { 1.1115, 0.0228, 0.45, 4.0 },
+[AVCOL_TRC_IEC61966_2_1] = { 1.055, 0.0031308, 1.0 / 2.4, 12.92 },
 [AVCOL_TRC_BT2020_10] = { 1.099,  0.018,  0.45, 4.5 },
 [AVCOL_TRC_BT2020_12] = { 1.0993, 0.0181, 0.45, 4.5 },
 };
@@ -1067,6 +1068,8 @@ static const AVOption colorspace_options[] = {
 ENUM("gamma28",  AVCOL_TRC_GAMMA28,  "trc"),
 ENUM("smpte170m",AVCOL_TRC_SMPTE170M,"trc"),
 ENUM("smpte240m",AVCOL_TRC_SMPTE240M,"trc"),
+ENUM("srgb", AVCOL_TRC_IEC61966_2_1, "trc"),
+ENUM("iec61966-2-1", AVCOL_TRC_IEC61966_2_1, "trc"),
 ENUM("bt2020-10",AVCOL_TRC_BT2020_10,"trc"),
 ENUM("bt2020-12",AVCOL_TRC_BT2020_12,"trc"),
 
-- 
2.10.0

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

Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range

[Vittorio Giovara]

On Mon, Sep 19, 2016 at 8:36 AM, Ronald S. Bultje  wrote:
> Hi,
>
> On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch  wrote:
>>
>> On Fri, Sep 16, 2016 at 03:20:49PM +0200, Vittorio Giovara wrote:
>> > This is the assumption that is made in pixel format conversion do
>> > throughout the code (in particular swscale), and BT-specifications
>> > mandate.
>> >
>> > Add a warning to inform the user that an automatic selection is being
>> > made.
>> >
>> > Signed-off-by: Vittorio Giovara 
>> > ---
>> >  libavfilter/vf_colorspace.c | 9 ++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
>> > index e69be50..41d1692 100644
>> > --- a/libavfilter/vf_colorspace.c
>> > +++ b/libavfilter/vf_colorspace.c
>> > @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void
>> > *data, int job_nr, int n_jobs)
>> >  return 0;
>> >  }
>> >
>> > -static int get_range_off(int *off, int *y_rng, int *uv_rng,
>> > +static int get_range_off(AVFilterContext *ctx, int *off,
>> > + int *y_rng, int *uv_rng,
>> >   enum AVColorRange rng, int depth)
>> >  {
>> >  switch (rng) {
>> > +case AVCOL_RANGE_UNSPECIFIED:
>> > +av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming
>> > tv/mpeg\n");

Hey Ronald,
this message ends up spamming quite a bit, since it prints a warning
for every frame, which is a tad excessive.

As mentioned in this thread, I doubt this line could be beneficial to
the user, and needlessly filling the logs might be just worse since it
makes it harder to notice warnings in the first place. I don't think
it warrants a if_already_warned clause, would it be possible to just
drop it?

Thanks (please CC)
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] mov: Evaluate the movie display matrix

[Vittorio Giovara]

On Thu, Oct 13, 2016 at 6:50 PM, Vittorio Giovara
 wrote:
> This matrix needs to be applied after all others have (currently only
> display matrix from trak), but cannot be handled in movie box, since
> streams are not allocated yet. So store it in main context, and apply
> it when appropriate, that is after parsing the tkhd one.
>
> Signed-off-by: Vittorio Giovara 
> ---
> Reworked second matrix handling so that it is always applied, which makes
> the code easier to read and test. Updated according reviews, rolled back
> a couple of points for the reasons explained in the thread.
>
> Needs a new sample to be uploaded to fate,
> https://www.dropbox.com/s/qfio4bjhkpz3p4o/displaymatrix.mov?dl=0,
> the previous one can be deleted.
>
> Cheers,
> Vittorio
>
>  libavformat/isom.h|  2 ++
>  libavformat/mov.c | 53 
> +++
>  tests/fate/mov.mak|  6 -
>  tests/ref/fate/mov-display-matrix | 10 
>  4 files changed, 59 insertions(+), 12 deletions(-)
>  create mode 100644 tests/ref/fate/mov-display-matrix

Ping? Please cc me or i might miss replies.
Thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCHv3] mov: Evaluate the movie display matrix

[Vittorio Giovara]

This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet. So store it in main context, and apply
it when appropriate, that is after parsing the tkhd one.

Signed-off-by: Vittorio Giovara 
---
Reworked second matrix handling so that it is always applied, which makes
the code easier to read and test. Updated according reviews, rolled back
a couple of points for the reasons explained in the thread.

Needs a new sample to be uploaded to fate,
https://www.dropbox.com/s/qfio4bjhkpz3p4o/displaymatrix.mov?dl=0,
the previous one can be deleted.

Cheers,
Vittorio

 libavformat/isom.h|  2 ++
 libavformat/mov.c | 53 +++
 tests/fate/mov.mak|  6 -
 tests/ref/fate/mov-display-matrix | 10 
 4 files changed, 59 insertions(+), 12 deletions(-)
 create mode 100644 tests/ref/fate/mov-display-matrix

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 2246fed..2aeb8fa 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -238,6 +238,8 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index a15c8d1..e8da77f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3798,16 +3804,33 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 return 0;
 }
 
+// return 0 when matrix is identity, 1 otherwise
+#define IS_MATRIX_FULL(matrix)   \
+(matrix[0][0] != (1 << 16) ||\
+ matrix[1][1] != (1 << 16) ||\
+ matrix[2][2] != (1 << 30) ||\
+ matrix[0][1] || matrix[0][2] || \
+ matrix[1][0] || matrix[1][2] || \
+ matrix[2][0] || matrix[2][1])
+
+// fixed point to double
+#define CONV_FP(x, sh) ((double) (x)) / (1 << (sh))
+
+// double to fixed point
+#define CONV_DB(x, sh) ((int32_t) ((x) * (1 << (sh
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
+int res_display_matrix[3][3];
 AVStream *st;
 MOVStreamContext *sc;
 int version;
 int flags;
+double val = 0;
 
 if (c->fc->nb_streams < 1)
 return 0;
@@ -3853,15 +3876,22 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 sc->width = width >> 16;
 sc->height = height >> 16;
 
+// apply the moov display matrix
+for (i = 0; i < 3; i++) {
+for (j = 0; j < 3; j++) {
+int sh = j == 2 ? 30 : 16;
+for (e = 0; e < 3; e++) {
+val += CONV_FP(display_matrix[i][e], sh) *
+   CONV_FP(c->movie_display_matrix[e][j], sh);
+}
+res_display_matrix[i][j] = CONV_DB(val, sh);
+val = 0;
+}
+}
+
 // save the matrix and add rotate metadata when it is not the default
 // identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+if (IS_MATRIX_FULL(res_display_matrix)) {
 double rotate;
 
 av_freep(&sc->display_matrix);
@@ -3871,7 +3901,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 for (i = 0; i < 3; i++)
 for (j = 0; j < 3; j++)
-sc->display_matrix[i * 3 + j] = display_matrix[i][j];
+sc->display_matrix[i * 3 + j] = res_display_matrix[i][j];
 
 rotate = av_display_rota

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

On Sat, Oct 8, 2016 at 1:39 PM, Michael Niedermayer
 wrote:
> On Fri, Oct 07, 2016 at 10:38:22PM -0400, Vittorio Giovara wrote:
>> On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
>>  wrote:
>> > On Fri, Oct 07, 2016 at 03:31:46PM -0400, Vittorio Giovara wrote:
>> >> This matrix needs to be applied after all others have (currently only
>> >> display matrix from trak), but cannot be handled in movie box, since
>> >> streams are not allocated yet.
>> >>
>> >> So store it in main context and if not identity, apply it when 
>> >> appropriate,
>> >> handling the case when trak display matrix is identity and when it is not.
>> >>
>> >> Signed-off-by: Vittorio Giovara 
>> >> ---
>> >
>> >> +} else { // otherwise multiply the two and store the result
>> >> +int64_t val = 0;
>> >> +for (i = 0; i < 3; i++) {
>> >> +for (j = 0; j < 3; j++) {
>> >> +int sh = j == 2 ? 30 : 16;
>> >> +for (e = 0; e < 3; e++) {
>> >> +val += CONV_FP2INT(display_matrix[i][e], sh) *
>> >> +   
>> >> CONV_FP2INT(c->movie_display_matrix[e][j], sh);
>> >
>> > This does not work (you are dividing the 32bit numbers down to 2 bit)
>>
>> I don't understand this comment, are you referring to the fact that
>> the first two columns of the display matrix are 16.16, while the third
>> column is 2:30?
>
> you have 32bit integers and sh can be 30, in that case you throw away
> 30 of 32 bits before doing anything with the number

yes but that value will be between 0 and 0.99, which can be safely ignored.

>> > also its not tested by the fate testcase
>> > i can just replace it by 0 and fate-mov-movie-display-matrix still
>> > passes
>>
>> Yes, the sample I shared only has the movie display matrix, not
>> trak+movie. I can create another sample if you insist, but a fate test
>> would test little more than the matrix multiplication loops.
>
> yes

ok i'll simplify the code altogether

>> > the macros also lack some () protection giving probably unintended
>> > results
>>
>> Can you tell me how many more () would they need?
>
> #define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
>
> is missing at least 2 pairs
>
> #define CONV_FP2INT(x, sh) (((int64_t) (x)) / (1 << (sh)))

thanks, applied

> also the rounding is not optimal it should likely be rounding to
> nearest
> the division can be replaced by a shift while correcting the rounding

the rounding is fine, as i said the error is below 1 which is
acceptable for transform operations
also i really doubt using int64 is useful here, given that the numbers
are immediately converted to double when computing disp_transform and
hypot. I actually fear that using int64 will increase the error so I'm
tempted to drop it.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
 wrote:
> On Fri, Oct 07, 2016 at 03:31:46PM -0400, Vittorio Giovara wrote:
>> This matrix needs to be applied after all others have (currently only
>> display matrix from trak), but cannot be handled in movie box, since
>> streams are not allocated yet.
>>
>> So store it in main context and if not identity, apply it when appropriate,
>> handling the case when trak display matrix is identity and when it is not.
>>
>> Signed-off-by: Vittorio Giovara 
>> ---
>
>> +} else { // otherwise multiply the two and store the result
>> +int64_t val = 0;
>> +for (i = 0; i < 3; i++) {
>> +for (j = 0; j < 3; j++) {
>> +int sh = j == 2 ? 30 : 16;
>> +for (e = 0; e < 3; e++) {
>> +val += CONV_FP2INT(display_matrix[i][e], sh) *
>> +   CONV_FP2INT(c->movie_display_matrix[e][j], 
>> sh);
>
> This does not work (you are dividing the 32bit numbers down to 2 bit)

I don't understand this comment, are you referring to the fact that
the first two columns of the display matrix are 16.16, while the third
column is 2:30?

> also its not tested by the fate testcase
> i can just replace it by 0 and fate-mov-movie-display-matrix still
> passes

Yes, the sample I shared only has the movie display matrix, not
trak+movie. I can create another sample if you insist, but a fate test
would test little more than the matrix multiplication loops.

> the macros also lack some () protection giving probably unintended
> results

Can you tell me how many more () would they need?

From the subsequent email

> to explain a bit more how to do it with int64 instead of double floats
> int32 can be multiplied together to form int64 with a new fixed point
> without rounding or shifts
> then added up and then at the end shifted down with rounding to get
> back to the fixed point needed for the final destination

The problem is that the display matrix fields have different mantissa
length, the first two columns are 16.16, the last one is 2:30. I think
that method would work if all elements were the same length no?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet.

So store it in main context and if not identity, apply it when appropriate,
handling the case when trak display matrix is identity and when it is not.

Signed-off-by: Vittorio Giovara 
---
Updated according review.
Vittorio

 libavformat/isom.h  |  2 ++
 libavformat/mov.c   | 63 +++--
 tests/fate/mov.mak  |  6 +++-
 tests/ref/fate/mov-movie-display-matrix | 10 ++
 4 files changed, 70 insertions(+), 11 deletions(-)
 create mode 100644 tests/ref/fate/mov-movie-display-matrix

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 2246fed..2aeb8fa 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -238,6 +238,8 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index a15c8d1..307ce08 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3798,9 +3804,24 @@ static int mov_read_meta(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 return 0;
 }
 
+// return 0 when matrix is identity, 1 otherwise
+#define IS_MATRIX_FULL(matrix)   \
+(matrix[0][0] != (1 << 16) ||\
+ matrix[1][1] != (1 << 16) ||\
+ matrix[2][2] != (1 << 30) ||\
+ matrix[0][1] || matrix[0][2] || \
+ matrix[1][0] || matrix[1][2] || \
+ matrix[2][0] || matrix[2][1])
+
+// fixed point to int64_t
+#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
+
+// int64_t to fixed point
+#define CONV_INT2FP(x, sh) (int32_t) ((x) * (1 << sh))
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
@@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 // save the matrix and add rotate metadata when it is not the default
 // identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+if (IS_MATRIX_FULL(display_matrix)) {
 double rotate;
 
 av_freep(&sc->display_matrix);
@@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 }
 }
 
+// if movie display matrix is not identity, and if this is a video track
+if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
+// if trak display matrix was identity, just copy the movie one
+if (!sc->display_matrix) {
+sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
+if (!sc->display_matrix)
+return AVERROR(ENOMEM);
+
+for (i = 0; i < 3; i++)
+for (j = 0; j < 3; j++)
+sc->display_matrix[i * 3 + j] = 
c->movie_display_matrix[i][j];
+} else { // otherwise multiply the two and store the result
+int64_t val = 0;
+for (i = 0; i < 3; i++) {
+for (j = 0; j < 3; j++) {
+int sh = j == 2 ? 30 : 16;
+for (e = 0; e < 3; e++) {
+val += CONV_FP2INT(display_matrix[i][e], sh) *
+   CONV_FP2INT(c->movie_display_matrix[e][j], sh);
+}
+sc->display_matrix[i * 3 + j] = CONV_INT2FP(val, sh);
+val = 0;
+}
+}
+}
+}

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

On Thu, Oct 6, 2016 at 4:07 PM, Michael Niedermayer
 wrote:
> On Thu, Oct 06, 2016 at 12:12:07AM -0400, Vittorio Giovara wrote:
>> On Wed, Oct 5, 2016 at 10:07 PM, Michael Niedermayer
>>  wrote:
>> > On Wed, Oct 05, 2016 at 05:25:33PM -0400, Vittorio Giovara wrote:
>> >> This matrix needs to be applied after all others have (currently only
>> >> display matrix from trak), but cannot be handled in movie box, since
>> >> streams are not allocated yet.
>> >>
>> >> So store it in main context and if not identity, apply it when 
>> >> appropriate,
>> >> handling the case when trak display matrix is identity and when it is not.
>> >
>> > do you have a testcase for this which you can share ?
>> >
>> > thx
>> >
>> > [...]
>> > --
>>
>> I created one for the occasion
>> https://www.dropbox.com/s/w2uu37o11rvoz1q/moviedispmat.mp4?dl=1
>
> with ffplay before the patch the image looks round, after the patch
> it looks quite squished, is that intended ?

yes that is correct

> what is the intended / correct SAR / DAR for this sample ?

without the patch Stream #0:0(und): Video: h264 (High) (avc1 /
0x31637661), yuv420p, 540x576 [SAR 1:1 DAR 15:16], 102 kb/s, 25 fps,
25 tbr, 12800 tbn, 50 tbc (default)

with the patch Stream #0:0(und): Video: h264 (High) (avc1 /
0x31637661), yuv420p, 540x576 [SAR 1:1 DAR 15:16], 102 kb/s, SAR
93207:65536 DAR 1016804:762601, 25 fps, 25 tbr, 12800 tbn, 50 tbc
(default)

This last one is the intended and correct one.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

On Wed, Oct 5, 2016 at 10:07 PM, Michael Niedermayer
 wrote:
> On Wed, Oct 05, 2016 at 05:25:33PM -0400, Vittorio Giovara wrote:
>> This matrix needs to be applied after all others have (currently only
>> display matrix from trak), but cannot be handled in movie box, since
>> streams are not allocated yet.
>>
>> So store it in main context and if not identity, apply it when appropriate,
>> handling the case when trak display matrix is identity and when it is not.
>
> do you have a testcase for this which you can share ?
>
> thx
>
> [...]
> --

I created one for the occasion
https://www.dropbox.com/s/w2uu37o11rvoz1q/moviedispmat.mp4?dl=1
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet.

So store it in main context and if not identity, apply it when appropriate,
handling the case when trak display matrix is identity and when it is not.

Signed-off-by: Vittorio Giovara 
---
Please keep my in CC.
Vittorio

 libavformat/isom.h |  2 ++
 libavformat/mov.c  | 63 +-
 2 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 2246fed..2aeb8fa 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -238,6 +238,8 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index a15c8d1..26b332c 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3798,9 +3804,24 @@ static int mov_read_meta(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 return 0;
 }
 
+// return 0 when matrix is identity, 1 otherwise
+#define IS_MATRIX_FULL(matrix)   \
+(matrix[0][0] != (1 << 16) ||\
+ matrix[1][1] != (1 << 16) ||\
+ matrix[2][2] != (1 << 30) ||\
+ matrix[0][1] || matrix[0][2] || \
+ matrix[1][0] || matrix[1][2] || \
+ matrix[2][0] || matrix[2][1])
+
+// fixed point to double
+#define CONV_FP(x, sh) ((double) (x)) / (1 << sh)
+
+// double to fixed point
+#define CONV_DB(x, sh) (int32_t) ((x) * (1 << sh))
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
@@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 // save the matrix and add rotate metadata when it is not the default
 // identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+if (IS_MATRIX_FULL(display_matrix)) {
 double rotate;
 
 av_freep(&sc->display_matrix);
@@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 }
 }
 
+// if movie display matrix is not identity, and if this is a video track
+if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
+// if trak display matrix was identity, just copy the movie one
+if (!sc->display_matrix) {
+sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
+if (!sc->display_matrix)
+return AVERROR(ENOMEM);
+
+for (i = 0; i < 3; i++)
+for (j = 0; j < 3; j++)
+sc->display_matrix[i * 3 + j] = 
c->movie_display_matrix[i][j];
+} else { // otherwise multiply the two and store the result
+double val = 0;
+for (i = 0; i < 3; i++) {
+for (j = 0; j < 3; j++) {
+int sh = j == 2 ? 30 : 16;
+for (e = 0; e < 3; e++) {
+val += CONV_FP(display_matrix[i][e], sh) *
+   CONV_FP(c->movie_display_matrix[e][j], sh);
+}
+sc->display_matrix[i * 3 + j] = CONV_DB(val, sh);
+val = 0;
+}
+}
+}
+}
+
 // transform the display width/height according to the matrix
 // to keep the same scale, use [width height 1<<16]
 if (width && height && sc->display_matrix) {
 

[FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range

[Vittorio Giovara]

This is the assumption that is made in pixel format conversion do
throughout the code (in particular swscale), and BT-specifications
mandate.

Add a warning to inform the user that an automatic selection is being
made.

Signed-off-by: Vittorio Giovara 
---
 libavfilter/vf_colorspace.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index e69be50..41d1692 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void *data, int 
job_nr, int n_jobs)
 return 0;
 }
 
-static int get_range_off(int *off, int *y_rng, int *uv_rng,
+static int get_range_off(AVFilterContext *ctx, int *off,
+ int *y_rng, int *uv_rng,
  enum AVColorRange rng, int depth)
 {
 switch (rng) {
+case AVCOL_RANGE_UNSPECIFIED:
+av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming tv/mpeg\n");
 case AVCOL_RANGE_MPEG:
 *off = 16 << (depth - 8);
 *y_rng = 219 << (depth - 8);
@@ -740,7 +743,7 @@ static int create_filtergraph(AVFilterContext *ctx,
 double rgb2yuv[3][3], (*yuv2rgb)[3] = s->yuv2rgb_dbl_coeffs;
 int off, bits, in_rng;
 
-res = get_range_off(&off, &s->in_y_rng, &s->in_uv_rng,
+res = get_range_off(ctx, &off, &s->in_y_rng, &s->in_uv_rng,
 s->in_rng, in_desc->comp[0].depth);
 if (res < 0) {
 av_log(ctx, AV_LOG_ERROR,
@@ -773,7 +776,7 @@ static int create_filtergraph(AVFilterContext *ctx,
 double (*rgb2yuv)[3] = s->rgb2yuv_dbl_coeffs;
 int off, out_rng, bits;
 
-res = get_range_off(&off, &s->out_y_rng, &s->out_uv_rng,
+res = get_range_off(ctx, &off, &s->out_y_rng, &s->out_uv_rng,
 s->out_rng, out_desc->comp[0].depth);
 if (res < 0) {
 av_log(ctx, AV_LOG_ERROR,
-- 
2.9.3

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

Re: [FFmpeg-devel] [PATCH 1/2] vf_colorspace: Interpret unspecified color range as limited range

[Vittorio Giovara]

> On Sep 15, 2016, at 2:45 PM, Ronald S. Bultje  wrote:
> 
> Hi Vittorio,
> 
>> On Thu, Sep 15, 2016 at 8:33 AM, Vittorio Giovara 
>>  wrote:
>> I agree we should always be very very careful about automatic assignments, 
>> although this one seems pretty safe.
>> 
>> I don't think it's necessary, but if you prefer I can add a warning whenever 
>> range is not specified (even though it might trigger a few false positives).
> 
> Which false positive are you afraid of?

By "false positive" I mean only the fact that it might needlessly print a 
warning, and the user might be confused by it.
Vittorio 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] vf_colorspace: Interpret unspecified color range as limited range

[Vittorio Giovara]

> Hi, On Wed, Sep 14, 2016 at 5:09 PM, Vittorio Giovara < vittorio.giovara at 
> gmail.com> wrote: > This is the assumption that is made in pixel format 
> conversion do > throughout the code (in particular swscale and 
> vf_colormatrix). > > Signed-off-by: Vittorio Giovara  gmail.com> > --- > libavfilter/vf_colorspace.c | 1 + > 1 file changed, 1 
> insertion(+) > > diff --git a/libavfilter/vf_colorspace.c 
> b/libavfilter/vf_colorspace.c > index b9ecb5f..7e87cd8 100644 > --- 
> a/libavfilter/vf_colorspace.c > +++ b/libavfilter/vf_colorspace.c > @@ -522,6 
> +522,7 @@ static int get_range_off(int *off, int *y_rng, int > *uv_rng, > 
> enum AVColorRange rng, int depth) > { > switch (rng) { > + case 
> AVCOL_RANGE_UNSPECIFIED: >  case AVCOL_RANGE_MPEG: > *off = 16 << (depth 
> - 8); > *y_rng = 219 << (depth - 8); > -- > 2.9.3   I'm open to this, but let 
> me explain why I didn't follow the convention here: the convention is wrong, 
> or rather, the convention is outdated. Classically, for low-resolution video, 
> this is indeed true. But for modern video (bt2020 etc., probably even smpte*) 
> at larger resolutions, I feel the default should be pc/jpeg range, not 
> tv/mpeg range, because that's usually what it is.
Hi Ronald,
I am not sure about this point, unless I am missing something.

I've checked on the bt2020 spec and the nominal range described in table 5 
shows 64-960 for 10bit which would match the limited representation.

Also bt2100 says that "full range" is "newly introduced" and should not be used 
unless "all parties agree", meaning range needs of be explicitly marked as full.
> I was somewhat tempted to not make automatic decisions that could have a 
> fairly substantial negative effect on the end result (without informing the 
> end user about it!). Asking the user to explicitly specify the pixel range is 
> one way of informing. (You could also add a av_log(.., WARNING, ..) if the 
> rng is UNSPECIFIED.) But if others think this is the best solution, I'm OK 
> with it. Ronald
I agree we should always be very very careful about automatic assignments, 
although this one seems pretty safe.

I don't think it's necessary, but if you prefer I can add a warning whenever 
range is not specified (even though it might trigger a few false positives).

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

[FFmpeg-devel] [PATCH 1/2] vf_colorspace: Interpret unspecified color range as limited range

[Vittorio Giovara]

This is the assumption that is made in pixel format conversion do
throughout the code (in particular swscale and vf_colormatrix).

Signed-off-by: Vittorio Giovara 
---
 libavfilter/vf_colorspace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index b9ecb5f..7e87cd8 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -522,6 +522,7 @@ static int get_range_off(int *off, int *y_rng, int *uv_rng,
  enum AVColorRange rng, int depth)
 {
 switch (rng) {
+case AVCOL_RANGE_UNSPECIFIED:
 case AVCOL_RANGE_MPEG:
 *off = 16 << (depth - 8);
 *y_rng = 219 << (depth - 8);
-- 
2.9.3

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

[FFmpeg-devel] [PATCH 2/2] vf_colorspace: Add modern names for color range option

[Vittorio Giovara]

Allows to use values returned from API and from ffprobe directly.

Signed-off-by: Vittorio Giovara 
---
 libavfilter/vf_colorspace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 7e87cd8..b993928 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -1039,7 +1039,9 @@ static const AVOption colorspace_options[] = {
 { "range",  "Output color range",
   OFFSET(user_rng),   AV_OPT_TYPE_INT, { .i64 = AVCOL_RANGE_UNSPECIFIED },
   AVCOL_RANGE_UNSPECIFIED, AVCOL_RANGE_NB - 1, FLAGS, "rng" },
+ENUM("tv",  AVCOL_RANGE_MPEG,  "rng"),
 ENUM("mpeg",AVCOL_RANGE_MPEG,  "rng"),
+ENUM("pc",  AVCOL_RANGE_JPEG,  "rng"),
 ENUM("jpeg",AVCOL_RANGE_JPEG,  "rng"),
 
 { "primaries",  "Output color primaries",
-- 
2.9.3

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

[FFmpeg-devel] [PATCH] vf_colorspace: Add BT-names for gamma22/28 transfer option

[Vittorio Giovara]

Allows to use values returned from API and from ffprobe directly.

Signed-off-by: Vittorio Giovara 
---
 libavfilter/vf_colorspace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 45fd917..b9ecb5f 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -1055,7 +1055,9 @@ static const AVOption colorspace_options[] = {
   OFFSET(user_trc),   AV_OPT_TYPE_INT, { .i64 = AVCOL_TRC_UNSPECIFIED },
   AVCOL_TRC_RESERVED0, AVCOL_TRC_NB - 1, FLAGS, "trc" },
 ENUM("bt709",AVCOL_TRC_BT709,"trc"),
+ENUM("bt470m",   AVCOL_TRC_GAMMA22,  "trc"),
 ENUM("gamma22",  AVCOL_TRC_GAMMA22,  "trc"),
+ENUM("bt470bg",  AVCOL_TRC_GAMMA28,  "trc"),
 ENUM("gamma28",  AVCOL_TRC_GAMMA28,  "trc"),
 ENUM("smpte170m",AVCOL_TRC_SMPTE170M,"trc"),
 ENUM("smpte240m",AVCOL_TRC_SMPTE240M,"trc"),
-- 
2.9.3

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

[FFmpeg-devel] [PATCHv3] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

The filter needs input frames with color properties filled out by
the decoder. Since this is not always possible, add input options to
the filter so that user may override color space, color primaries,
transfer characteristics, and color range, as well as a generic option
to set all properties at once.

Signed-off-by: Vittorio Giovara 
---
Options moved at the bottom of the list as requested.
Vittorio

 doc/filters.texi| 20 
 libavfilter/vf_colorspace.c | 40 +++-
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/doc/filters.texi b/doc/filters.texi
index c12b093..00ec1ea 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -5235,6 +5235,7 @@ Convert colorspace, transfer characteristics or color 
primaries.
 The filter accepts the following options:
 
 @table @option
+@anchor{all}
 @item all
 Specify all color properties at once.
 
@@ -5266,6 +5267,7 @@ BT.2020
 
 @end table
 
+@anchor{space}
 @item space
 Specify output colorspace.
 
@@ -5291,6 +5293,7 @@ BT.2020 with non-constant luminance
 
 @end table
 
+@anchor{trc}
 @item trc
 Specify output transfer characteristics.
 
@@ -5319,6 +5322,7 @@ BT.2020 for 12-bits content
 
 @end table
 
+@anchor{primaries}
 @item primaries
 Specify output color primaries.
 
@@ -5344,6 +5348,7 @@ BT.2020
 
 @end table
 
+@anchor{range}
 @item range
 Specify output color range.
 
@@ -5423,6 +5428,21 @@ von Kries whitepoint adaptation
 identity whitepoint adaptation (i.e. no whitepoint adaptation)
 @end table
 
+@item iall
+Override all input properties at once. Same accepted values as @ref{all}.
+
+@item ispace
+Override input colorspace. Same accepted values as @ref{space}.
+
+@item iprimaries
+Override input color primaries. Same accepted values as @ref{primaries}.
+
+@item itrc
+Override input transfer characteristics. Same accepted values as @ref{trc}.
+
+@item irange
+Override input color range. Same accepted values as @ref{range}.
+
 @end table
 
 The filter converts the transfer characteristics, color space and color
diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index e4022f8..45fd917 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -128,11 +128,11 @@ typedef struct ColorSpaceContext {
 
 ColorSpaceDSPContext dsp;
 
-enum Colorspace user_all;
-enum AVColorSpace in_csp, out_csp, user_csp;
-enum AVColorRange in_rng, out_rng, user_rng;
-enum AVColorTransferCharacteristic in_trc, out_trc, user_trc;
-enum AVColorPrimaries in_prm, out_prm, user_prm;
+enum Colorspace user_all, user_iall;
+enum AVColorSpace in_csp, out_csp, user_csp, user_icsp;
+enum AVColorRange in_rng, out_rng, user_rng, user_irng;
+enum AVColorTransferCharacteristic in_trc, out_trc, user_trc, user_itrc;
+enum AVColorPrimaries in_prm, out_prm, user_prm, user_iprm;
 enum AVPixelFormat in_format, user_format;
 int fast_mode;
 enum DitherMode dither;
@@ -581,6 +581,10 @@ static int create_filtergraph(AVFilterContext *ctx,
 
 if (!s->out_primaries || !s->in_primaries) {
 s->in_prm = in->color_primaries;
+if (s->user_iall != CS_UNSPECIFIED)
+s->in_prm = default_prm[FFMIN(s->user_iall, CS_NB)];
+if (s->user_iprm != AVCOL_PRI_UNSPECIFIED)
+s->in_prm = s->user_iprm;
 s->in_primaries = get_color_primaries(s->in_prm);
 if (!s->in_primaries) {
 av_log(ctx, AV_LOG_ERROR,
@@ -638,6 +642,10 @@ static int create_filtergraph(AVFilterContext *ctx,
 if (!s->in_txchr) {
 av_freep(&s->lin_lut);
 s->in_trc = in->color_trc;
+if (s->user_iall != CS_UNSPECIFIED)
+s->in_trc = default_trc[FFMIN(s->user_iall, CS_NB)];
+if (s->user_itrc != AVCOL_TRC_UNSPECIFIED)
+s->in_trc = s->user_itrc;
 s->in_txchr = get_transfer_characteristics(s->in_trc);
 if (!s->in_txchr) {
 av_log(ctx, AV_LOG_ERROR,
@@ -680,7 +688,13 @@ static int create_filtergraph(AVFilterContext *ctx,
 
 if (!s->in_lumacoef) {
 s->in_csp = in->colorspace;
+if (s->user_iall != CS_UNSPECIFIED)
+s->in_csp = default_csp[FFMIN(s->user_iall, CS_NB)];
+if (s->user_icsp != AVCOL_SPC_UNSPECIFIED)
+s->in_csp = s->user_icsp;
 s->in_rng = in->color_range;
+if (s->user_irng != AVCOL_RANGE_UNSPECIFIED)
+s->in_rng = s->user_irng;
 s->in_lumacoef = get_luma_coefficients(s->in_csp);
 if (!s->in_lumacoef) {
 av_log(ctx, AV_LOG_ERROR,
@@ -1078,6 +1092,22 @@ static const AVOption colorspace_options[] = {
 ENUM("vonkries", WP_ADAPT_VON_KRIES, "wpadapt"),
 ENUM("identity", WP_ADAPT_IDENTITY, "wpadapt"),
 
+{ "iall",   "Set

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

On Tue, Aug 30, 2016 at 10:54 AM, Paul B Mahol  wrote:
>
>
> On Tuesday, August 30, 2016, Vittorio Giovara 
> wrote:
>>
>> On Tue, Aug 30, 2016 at 4:13 AM, Paul B Mahol  wrote:
>> >
>> >
>> > On Mon, Aug 29, 2016 at 5:53 PM, Ronald S. Bultje 
>> > wrote:
>> >>
>> >> Hi,
>> >>
>> >> On Mon, Aug 29, 2016 at 11:23 AM, Vittorio Giovara <
>> >> vittorio.giov...@gmail.com> wrote:
>> >>
>> >> > The filter needs input frames with color properties filled out by
>> >> > the decoder. Since this is not always possible, add input options to
>> >> > the filter so that user may override color space, color primaries,
>> >> > transfer characteristics, and color range, as well as a generic
>> >> > option
>> >> > to set all properties at once.
>> >> >
>> >> > Signed-off-by: Vittorio Giovara 
>> >> > ---
>> >> > * added iall option
>> >> > * updated filter documentation
>> >> >
>> >> > Please CC.
>> >> > Vittorio
>> >> >
>> >> >  doc/filters.texi| 20 
>> >> >  libavfilter/vf_colorspace.c | 39
>> >> > ++-
>> >> >  2 files changed, 54 insertions(+), 5 deletions(-)
>> >>
>> >>
>> >> lgtm.
>> >>
>> >> (I wonder if the error message - if the input AVFrame has no
>> >> trc/rng/csp/prm - should be changed to reflect that you can override
>> >> them
>> >> using the added properties? This isn't a big deal but maybe someone
>> >> feels
>> >> that's important.)
>> >
>> >
>> > Still breaks backward compatibility.
>>
>> I fail to see how, can you provide more details?
>
>
>
> when doing options without typing option name, but just values, like:
> val0:val1:...

Oh I see, thanks. Isn't that syntax a little too fragile for this kind
of filter?
Or, from another perspective, is the filter old enough that backward
compatibility should matter?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

On Tue, Aug 30, 2016 at 4:13 AM, Paul B Mahol  wrote:
>
>
> On Mon, Aug 29, 2016 at 5:53 PM, Ronald S. Bultje 
> wrote:
>>
>> Hi,
>>
>> On Mon, Aug 29, 2016 at 11:23 AM, Vittorio Giovara <
>> vittorio.giov...@gmail.com> wrote:
>>
>> > The filter needs input frames with color properties filled out by
>> > the decoder. Since this is not always possible, add input options to
>> > the filter so that user may override color space, color primaries,
>> > transfer characteristics, and color range, as well as a generic option
>> > to set all properties at once.
>> >
>> > Signed-off-by: Vittorio Giovara 
>> > ---
>> > * added iall option
>> > * updated filter documentation
>> >
>> > Please CC.
>> > Vittorio
>> >
>> >  doc/filters.texi| 20 
>> >  libavfilter/vf_colorspace.c | 39
>> > ++-
>> >  2 files changed, 54 insertions(+), 5 deletions(-)
>>
>>
>> lgtm.
>>
>> (I wonder if the error message - if the input AVFrame has no
>> trc/rng/csp/prm - should be changed to reflect that you can override them
>> using the added properties? This isn't a big deal but maybe someone feels
>> that's important.)
>
>
> Still breaks backward compatibility.

I fail to see how, can you provide more details?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCHv2] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

The filter needs input frames with color properties filled out by
the decoder. Since this is not always possible, add input options to
the filter so that user may override color space, color primaries,
transfer characteristics, and color range, as well as a generic option
to set all properties at once.

Signed-off-by: Vittorio Giovara 
---
* added iall option
* updated filter documentation

Please CC.
Vittorio

 doc/filters.texi| 20 
 libavfilter/vf_colorspace.c | 39 ++-
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/doc/filters.texi b/doc/filters.texi
index b50d7a6..8045840 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -5235,6 +5235,7 @@ Convert colorspace, transfer characteristics or color 
primaries.
 The filter accepts the following options:
 
 @table @option
+@anchor{all}
 @item all
 Specify all color properties at once.
 
@@ -5266,6 +5267,10 @@ BT.2020
 
 @end table
 
+@item iall
+Override all input properties at once. Same accepted values as @ref{all}.
+
+@anchor{space}
 @item space
 Specify output colorspace.
 
@@ -5291,6 +5296,10 @@ BT.2020 with non-constant luminance
 
 @end table
 
+@item ispace
+Override input colorspace. Same accepted values as @ref{space}.
+
+@anchor{trc}
 @item trc
 Specify output transfer characteristics.
 
@@ -5319,6 +5328,10 @@ BT.2020 for 12-bits content
 
 @end table
 
+@item itrc
+Override input transfer characteristics. Same accepted values as @ref{trc}.
+
+@anchor{primaries}
 @item primaries
 Specify output color primaries.
 
@@ -5344,6 +5357,10 @@ BT.2020
 
 @end table
 
+@item iprimaries
+Override input color primaries. Same accepted values as @ref{primaries}.
+
+@anchor{range}
 @item range
 Specify output color range.
 
@@ -5357,6 +5374,9 @@ JPEG (full) range
 
 @end table
 
+@item irange
+Override input color range. Same accepted values as @ref{range}.
+
 @item format
 Specify output color format.
 
diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index e4022f8..c1ef5b3 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -128,11 +128,11 @@ typedef struct ColorSpaceContext {
 
 ColorSpaceDSPContext dsp;
 
-enum Colorspace user_all;
-enum AVColorSpace in_csp, out_csp, user_csp;
-enum AVColorRange in_rng, out_rng, user_rng;
-enum AVColorTransferCharacteristic in_trc, out_trc, user_trc;
-enum AVColorPrimaries in_prm, out_prm, user_prm;
+enum Colorspace user_all, user_iall;
+enum AVColorSpace in_csp, out_csp, user_csp, user_icsp;
+enum AVColorRange in_rng, out_rng, user_rng, user_irng;
+enum AVColorTransferCharacteristic in_trc, out_trc, user_trc, user_itrc;
+enum AVColorPrimaries in_prm, out_prm, user_prm, user_iprm;
 enum AVPixelFormat in_format, user_format;
 int fast_mode;
 enum DitherMode dither;
@@ -581,6 +581,10 @@ static int create_filtergraph(AVFilterContext *ctx,
 
 if (!s->out_primaries || !s->in_primaries) {
 s->in_prm = in->color_primaries;
+if (s->user_iall != CS_UNSPECIFIED)
+s->in_prm = default_prm[FFMIN(s->user_iall, CS_NB)];
+if (s->user_iprm != AVCOL_PRI_UNSPECIFIED)
+s->in_prm = s->user_iprm;
 s->in_primaries = get_color_primaries(s->in_prm);
 if (!s->in_primaries) {
 av_log(ctx, AV_LOG_ERROR,
@@ -638,6 +642,10 @@ static int create_filtergraph(AVFilterContext *ctx,
 if (!s->in_txchr) {
 av_freep(&s->lin_lut);
 s->in_trc = in->color_trc;
+if (s->user_iall != CS_UNSPECIFIED)
+s->in_trc = default_trc[FFMIN(s->user_iall, CS_NB)];
+if (s->user_itrc != AVCOL_TRC_UNSPECIFIED)
+s->in_trc = s->user_itrc;
 s->in_txchr = get_transfer_characteristics(s->in_trc);
 if (!s->in_txchr) {
 av_log(ctx, AV_LOG_ERROR,
@@ -680,7 +688,13 @@ static int create_filtergraph(AVFilterContext *ctx,
 
 if (!s->in_lumacoef) {
 s->in_csp = in->colorspace;
+if (s->user_iall != CS_UNSPECIFIED)
+s->in_csp = default_csp[FFMIN(s->user_iall, CS_NB)];
+if (s->user_icsp != AVCOL_SPC_UNSPECIFIED)
+s->in_csp = s->user_icsp;
 s->in_rng = in->color_range;
+if (s->user_irng != AVCOL_RANGE_UNSPECIFIED)
+s->in_rng = s->user_irng;
 s->in_lumacoef = get_luma_coefficients(s->in_csp);
 if (!s->in_lumacoef) {
 av_log(ctx, AV_LOG_ERROR,
@@ -1002,6 +1016,9 @@ static const AVOption colorspace_options[] = {
 { "all","Set all color properties together",
   OFFSET(user_all),   AV_OPT_TYPE_INT, { .i64 = CS_UNSPECIFIED },
   CS_UNSPECIFIED, CS_NB - 1, FLAGS, "all" },
+{ "iall",   "Set all input color properties together",
+   

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

On Fri, Aug 26, 2016 at 5:16 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Fri, Aug 26, 2016 at 2:40 PM, Vittorio Giovara
>  wrote:
>>
>> On Fri, Aug 26, 2016 at 12:59 AM, Ronald S. Bultje 
>> wrote:
>> > Hi Vittorio,
>> >
>> > On Thu, Aug 25, 2016 at 7:14 PM, Vittorio Giovara
>> >  wrote:
>> >>
>> >> The filter needs input frames with color properties filled out by
>> >> the decoder. Since this is not always possible, add input options to
>> >> the filter so that user may override color space, color primaries,
>> >> transfer characteristics, and color range.
>> >>
>> >> Signed-off-by: Vittorio Giovara 
>> >> ---
>> >> Please keep me in CC.
>> >> Vittorio
>> >
>> > [..]
>> >
>> > Do you think it makes sense to have a "iall" property, similar to "all"?
>> > iprm/irange/itr/iprimeries would take precedence over iall, but iall
>> > could
>> > be used to set all 4 at once in the (common) case where they are
>> > consistent.
>>
>> Hi Ronald,
>> I had thought of that, but I am not sure.
>>
>> In order to "guess" the input parameters one would need to rely on
>> either an external analyzer, which is going to report properties one
>> by one, or determine them via various heuristics: this latter case
>> might not be very precise, so one could decide to avoid performing
>> conversion altogether, or only perform it if some of the color
>> properties are already present (so that you'd only need to override
>> one or two).
>>
>> Also I found that it's hard to identify a common consistent format:
>> for example, my files are often tagged with a mixture of bt470bg,
>> bt470m or smpte formats, and none follow a sensible pattern (with the
>> exception of bt709). So overall I'd say this `iall` option is not
>> really need, but if open to other opinions too.
>
>
> "iall" indeed is not perfect, which is why the per-type properties exist
> also. But it works reliably for bt709, smpte170/240 and bt2020, afaik.
>
> What I mused on this subject earlier is that the "iall", like "all", would
> likely be not very useful for professional (prod) applications, but it would
> make "quick hack" uses of the filter relatively easy. It saves me typing a
> whole bunch of characters for something quick I'm testing, and as such it
> saves valuable developer time. For prod versions, of course you'd want to be
> as specific as possible and you likely wouldn't use it.
>
> Ronald

All right, I'll add it, thanks for review.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

On Fri, Aug 26, 2016 at 12:59 AM, Ronald S. Bultje  wrote:
> Hi Vittorio,
>
> On Thu, Aug 25, 2016 at 7:14 PM, Vittorio Giovara
>  wrote:
>>
>> The filter needs input frames with color properties filled out by
>> the decoder. Since this is not always possible, add input options to
>> the filter so that user may override color space, color primaries,
>> transfer characteristics, and color range.
>>
>> Signed-off-by: Vittorio Giovara 
>> ---
>> Please keep me in CC.
>> Vittorio
>
> [..]
>
> Do you think it makes sense to have a "iall" property, similar to "all"?
> iprm/irange/itr/iprimeries would take precedence over iall, but iall could
> be used to set all 4 at once in the (common) case where they are consistent.

Hi Ronald,
I had thought of that, but I am not sure.

In order to "guess" the input parameters one would need to rely on
either an external analyzer, which is going to report properties one
by one, or determine them via various heuristics: this latter case
might not be very precise, so one could decide to avoid performing
conversion altogether, or only perform it if some of the color
properties are already present (so that you'd only need to override
one or two).

Also I found that it's hard to identify a common consistent format:
for example, my files are often tagged with a mixture of bt470bg,
bt470m or smpte formats, and none follow a sensible pattern (with the
exception of bt709). So overall I'd say this `iall` option is not
really need, but if open to other opinions too.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

On Fri, Aug 26, 2016 at 3:52 AM, Paul B Mahol  wrote:
> On 8/26/16, Vittorio Giovara  wrote:
>> The filter needs input frames with color properties filled out by
>> the decoder. Since this is not always possible, add input options to
>> the filter so that user may override color space, color primaries,
>> transfer characteristics, and color range.
>>
>> Signed-off-by: Vittorio Giovara 
>> ---
>> Please keep me in CC.
>> Vittorio
>>
>>  libavfilter/vf_colorspace.c | 28 
>>  1 file changed, 24 insertions(+), 4 deletions(-)
>
> Generally, new options should be added after old ones.

I added closer to the original options because they share exactly the
same input values, and seemed to logically belong there: in case an
option is added to or removed from the group, the developer can
immediately notice that the change is going to affect both options.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/3] vf_colorspace: Check av_frame_copy_props() return value

[Vittorio Giovara]

This function can potentially allocate memory.
---
Please keep me in CC.
Vittorio

 libavfilter/vf_colorspace.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 3d39f13..bf51c83 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -861,7 +861,11 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
 av_frame_free(&in);
 return AVERROR(ENOMEM);
 }
-av_frame_copy_props(out, in);
+res = av_frame_copy_props(out, in);
+if (res < 0) {
+av_frame_free(&in);
+return res;
+}
 
 out->color_primaries = s->user_prm == AVCOL_PRI_UNSPECIFIED ?
default_prm[FFMIN(s->user_all, CS_NB)] : 
s->user_prm;
-- 
2.9.3

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

[FFmpeg-devel] [PATCH 2/3] vf_colorspace: Add support for full range yuv

[Vittorio Giovara]

Whenever a full range video is input, since the YUVJ* formats are not
listed as supported for this filter, a range reduction takes place
through the auto-inserted format filter, forcing the conversion to
operate on a limited range,

However the filter handles full range videos perfectly fine, so adding
support to YUVJ* formats will allow skipping a conversion step, while
providing completely identical results.

Signed-off-by: Vittorio Giovara 
---
Please keep me in CC.
Vittorio

 libavfilter/vf_colorspace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index bf51c83..37e77d1 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -960,6 +960,7 @@ static int query_formats(AVFilterContext *ctx)
 AV_PIX_FMT_YUV420P,   AV_PIX_FMT_YUV422P,   AV_PIX_FMT_YUV444P,
 AV_PIX_FMT_YUV420P10, AV_PIX_FMT_YUV422P10, AV_PIX_FMT_YUV444P10,
 AV_PIX_FMT_YUV420P12, AV_PIX_FMT_YUV422P12, AV_PIX_FMT_YUV444P12,
+AV_PIX_FMT_YUVJ420P,  AV_PIX_FMT_YUVJ422P,  AV_PIX_FMT_YUVJ444P,
 AV_PIX_FMT_NONE
 };
 int res;
-- 
2.9.3

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

[FFmpeg-devel] [PATCH 3/3] vf_colorspace: Allow overriding input color properties

[Vittorio Giovara]

The filter needs input frames with color properties filled out by
the decoder. Since this is not always possible, add input options to
the filter so that user may override color space, color primaries,
transfer characteristics, and color range.

Signed-off-by: Vittorio Giovara 
---
Please keep me in CC.
Vittorio

 libavfilter/vf_colorspace.c | 28 
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index 37e77d1..8ef362e 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -129,10 +129,10 @@ typedef struct ColorSpaceContext {
 ColorSpaceDSPContext dsp;
 
 enum Colorspace user_all;
-enum AVColorSpace in_csp, out_csp, user_csp;
-enum AVColorRange in_rng, out_rng, user_rng;
-enum AVColorTransferCharacteristic in_trc, out_trc, user_trc;
-enum AVColorPrimaries in_prm, out_prm, user_prm;
+enum AVColorSpace in_csp, out_csp, user_csp, user_icsp;
+enum AVColorRange in_rng, out_rng, user_rng, user_irng;
+enum AVColorTransferCharacteristic in_trc, out_trc, user_trc, user_itrc;
+enum AVColorPrimaries in_prm, out_prm, user_prm, user_iprm;
 enum AVPixelFormat in_format, user_format;
 int fast_mode;
 enum DitherMode dither;
@@ -581,6 +581,8 @@ static int create_filtergraph(AVFilterContext *ctx,
 
 if (!s->out_primaries || !s->in_primaries) {
 s->in_prm = in->color_primaries;
+if (s->user_iprm != AVCOL_PRI_UNSPECIFIED)
+s->in_prm = s->user_iprm;
 s->in_primaries = get_color_primaries(s->in_prm);
 if (!s->in_primaries) {
 av_log(ctx, AV_LOG_ERROR,
@@ -638,6 +640,8 @@ static int create_filtergraph(AVFilterContext *ctx,
 if (!s->in_txchr) {
 av_freep(&s->lin_lut);
 s->in_trc = in->color_trc;
+if (s->user_itrc != AVCOL_TRC_UNSPECIFIED)
+s->in_trc = s->user_itrc;
 s->in_txchr = get_transfer_characteristics(s->in_trc);
 if (!s->in_txchr) {
 av_log(ctx, AV_LOG_ERROR,
@@ -680,7 +684,11 @@ static int create_filtergraph(AVFilterContext *ctx,
 
 if (!s->in_lumacoef) {
 s->in_csp = in->colorspace;
+if (s->user_icsp != AVCOL_SPC_UNSPECIFIED)
+s->in_csp = s->user_icsp;
 s->in_rng = in->color_range;
+if (s->user_irng != AVCOL_RANGE_UNSPECIFIED)
+s->in_rng = s->user_irng;
 s->in_lumacoef = get_luma_coefficients(s->in_csp);
 if (!s->in_lumacoef) {
 av_log(ctx, AV_LOG_ERROR,
@@ -1014,6 +1022,9 @@ static const AVOption colorspace_options[] = {
 { "space",  "Output colorspace",
   OFFSET(user_csp),   AV_OPT_TYPE_INT, { .i64 = AVCOL_SPC_UNSPECIFIED },
   AVCOL_PRI_RESERVED0, AVCOL_PRI_NB - 1, FLAGS, "csp" },
+{ "ispace", "Input colorspace",
+  OFFSET(user_icsp),  AV_OPT_TYPE_INT, { .i64 = AVCOL_SPC_UNSPECIFIED },
+  AVCOL_PRI_RESERVED0, AVCOL_PRI_NB - 1, FLAGS, "csp" },
 ENUM("bt709",   AVCOL_SPC_BT709,   "csp"),
 ENUM("fcc", AVCOL_SPC_FCC, "csp"),
 ENUM("bt470bg", AVCOL_SPC_BT470BG, "csp"),
@@ -1024,12 +1035,18 @@ static const AVOption colorspace_options[] = {
 { "range",  "Output color range",
   OFFSET(user_rng),   AV_OPT_TYPE_INT, { .i64 = AVCOL_RANGE_UNSPECIFIED },
   AVCOL_RANGE_UNSPECIFIED, AVCOL_RANGE_NB - 1, FLAGS, "rng" },
+{ "irange", "Input color range",
+  OFFSET(user_irng),  AV_OPT_TYPE_INT, { .i64 = AVCOL_RANGE_UNSPECIFIED },
+  AVCOL_RANGE_UNSPECIFIED, AVCOL_RANGE_NB - 1, FLAGS, "rng" },
 ENUM("mpeg",AVCOL_RANGE_MPEG,  "rng"),
 ENUM("jpeg",AVCOL_RANGE_JPEG,  "rng"),
 
 { "primaries",  "Output color primaries",
   OFFSET(user_prm),   AV_OPT_TYPE_INT, { .i64 = AVCOL_PRI_UNSPECIFIED },
   AVCOL_PRI_RESERVED0, AVCOL_PRI_NB - 1, FLAGS, "prm" },
+{ "iprimaries", "Input color primaries",
+  OFFSET(user_iprm),  AV_OPT_TYPE_INT, { .i64 = AVCOL_PRI_UNSPECIFIED },
+  AVCOL_PRI_RESERVED0, AVCOL_PRI_NB - 1, FLAGS, "prm" },
 ENUM("bt709",AVCOL_PRI_BT709,  "prm"),
 ENUM("bt470m",   AVCOL_PRI_BT470M, "prm"),
 ENUM("bt470bg",  AVCOL_PRI_BT470BG,"prm"),
@@ -1040,6 +1057,9 @@ static const AVOption colorspace_options[] = {
 { "trc","Output transfer characteristics",
   OFFSET(user_trc),   AV_OPT_TYPE_INT, { .i64 = AVCOL_TRC_UNSPECIFIED },
   AVCOL_TRC_RESERVED0, AVCOL_TRC_NB - 1, FLA

Re: [FFmpeg-devel] [libav-devel] [PATCH] avcodec/cfhd: Fixes cfhd_odd.mov which has a resolution of 496x241

[Vittorio Giovara]

On Sat, Mar 5, 2016 at 1:06 PM, Kieran Kunhya  wrote:
> In this case container width/height is better however.
> Thanks to koda for the sample
> ---
>  libavcodec/cfhd.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
> index e37bef0..98f16d3 100644
> --- a/libavcodec/cfhd.c
> +++ b/libavcodec/cfhd.c
> @@ -467,6 +467,9 @@ static int cfhd_decode(AVCodecContext *avctx, void *data, 
> int *got_frame,
>  coeff_data += lowpass_width;
>  }
>
> +/* Align to mod-4 position to continue reading tags */
> +bytestream2_seek(&gb, bytestream2_tell(&gb) & 3, SEEK_CUR);
> +
>  /* Copy last line of coefficients if odd height */
>  if (lowpass_height & 1) {
>  memcpy(&coeff_data[lowpass_height * lowpass_width],
> --

Confirmed to work with the sample. Here is an addendum to avoid
displaying garbage (untested with other samples)

commit 658fdd157f12633006533bc67ffb3b0d44a198df
Author: Vittorio Giovara 
Date:   Sat Mar 5 13:32:54 2016 -0500

cfhd: Crop visible height

diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
index e37bef0..4d2e933 100644
--- a/libavcodec/cfhd.c
+++ b/libavcodec/cfhd.c
@@ -420,6 +420,8 @@ static int cfhd_decode(AVCodecContext *avctx, void
*data, int *got_frame,
 break;
 }
 planes = av_pix_fmt_count_planes(s->coded_format);
+} else if (tag == -85) {
+avctx->height = data;
 } else
 av_log(avctx, AV_LOG_DEBUG,  "Unknown tag %i data %x\n",
tag, data);


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

Re: [FFmpeg-devel] [libav-devel] [PATCHv3] avcodec: Cineform HD Decoder

[Vittorio Giovara]

On Thu, Jan 28, 2016 at 1:15 PM, Kieran Kunhya  wrote:
> On Thu, 28 Jan 2016 at 16:26 Vittorio Giovara 
> wrote:
>
>> On Mon, Jan 25, 2016 at 6:15 PM, Kieran Kunhya 
>> wrote:
>> >>> +{
>> >>> +CFHDContext *s = avctx->priv_data;
>> >>> +
>> >>> +avctx->pix_fmt = AV_PIX_FMT_YUV422P10;
>> >>
>> >> if the decoder supports multiple pixel formats it's better not to
>> >> initialize the pixel format here, and wait until decode(). Otherwise
>> >> it's going to cause a "parameters changed" warning and reinit any
>> >> previous filter chain.
>> >
>> > There are some samples which don't have a pixel format flagged and are
>> > implicitly AV_PIX_FMT_YUV422P10.
>>
>> I still think this should be put off until you get to decoding a
>> frame: having a filterchain reinitialization can be disastrous under
>> certain conditions.
>>
>>
> Not my problem to handle issues with applications.

sure, but to give another example if you ffprobe/avprobe a chfd sample
it will always report yuv422p10, while sometimes it's not. In cases
like this one I think it's better to have it decode one frame to get
the correct pixel format (until some proper parser apis replace this
way).

>> >>> +
>> >>> +end:
>> >>> +if (ret < 0)
>> >>> +return ret;
>> >>> +
>> >>> +*got_frame = 1;
>> >>> +return avpkt->size;
>> >>
>> >> avpkt->size - bytestream2_get_bytes_left(&gb) no?
>> >
>> > Guaranteed to read all tags.
>>
>> if all bytes have been read return 0 then
>>
>>
> eh?

sorry, never mind, I misread parts of the doc
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCHv3] avcodec: Cineform HD Decoder

[Vittorio Giovara]

On Mon, Jan 25, 2016 at 6:15 PM, Kieran Kunhya  wrote:
>>> +{
>>> +CFHDContext *s = avctx->priv_data;
>>> +
>>> +avctx->pix_fmt = AV_PIX_FMT_YUV422P10;
>>
>> if the decoder supports multiple pixel formats it's better not to
>> initialize the pixel format here, and wait until decode(). Otherwise
>> it's going to cause a "parameters changed" warning and reinit any
>> previous filter chain.
>
> There are some samples which don't have a pixel format flagged and are
> implicitly AV_PIX_FMT_YUV422P10.

I still think this should be put off until you get to decoding a
frame: having a filterchain reinitialization can be disastrous under
certain conditions.

>>> +
>>> +end:
>>> +if (ret < 0)
>>> +return ret;
>>> +
>>> +*got_frame = 1;
>>> +return avpkt->size;
>>
>> avpkt->size - bytestream2_get_bytes_left(&gb) no?
>
> Guaranteed to read all tags.

if all bytes have been read return 0 then
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCHv3] avcodec: Cineform HD Decoder

[Vittorio Giovara]

On Sun, Jan 24, 2016 at 7:34 PM, Kieran Kunhya  wrote:
> Decodes YUV 4:2:2 10-bit and RGB 12-bit files.
> Older files with more subbands, skips, Bayer, alpha not supported.
> Alpha requires addition of GBRAP12 pixel format.

Actually you could set AV_PIX_FMT_GBRAP16 and tweak the
bits_per_raw_sample, not sure about the repercussion on the users
though.

> ---
> +static av_cold int cfhd_decode_init(AVCodecContext *avctx)

is this function _decode or _init? or is it decoder_init? imho names
would be simpler with just _ scheme.

> +{
> +CFHDContext *s = avctx->priv_data;
> +
> +avctx->pix_fmt = AV_PIX_FMT_YUV422P10;

if the decoder supports multiple pixel formats it's better not to
initialize the pixel format here, and wait until decode(). Otherwise
it's going to cause a "parameters changed" warning and reinit any
previous filter chain.

> +avctx->bits_per_raw_sample = 10;
> +s->avctx   = avctx;
> +avctx->width   = 0;
> +avctx->height  = 0;

isn't the context mallocz anyway?

> +return ff_cfhd_init_vlcs(s);
> +}
> +

> +static inline void filter(int16_t *output, ptrdiff_t out_stride, int16_t 
> *low, ptrdiff_t low_stride,
> +  int16_t *high, ptrdiff_t high_stride, int len, 
> uint8_t clip)
> +{
> +int16_t tmp;
> +
> +int i;
> +for (i = 0; i < len; i++) {
> +if (i == 0) {
> +tmp = (11*low[0*low_stride] - 4*low[1*low_stride] + 
> low[2*low_stride] + 4) >> 3;
> +output[(2*i+0)*out_stride] = (tmp + high[0*high_stride]) >> 1;
> +if (clip)
> +output[(2*i+0)*out_stride] = 
> av_clip_uintp2(output[(2*i+0)*out_stride], clip);
> +
> +tmp = ( 5*low[0*low_stride] + 4*low[1*low_stride] - 
> low[2*low_stride] + 4) >> 3;
> +output[(2*i+1)*out_stride] = (tmp - high[0*high_stride]) >> 1;
> +if (clip)
> +output[(2*i+1)*out_stride] = 
> av_clip_uintp2(output[(2*i+1)*out_stride], clip);
> +}
> +else if (i == len-1){

nit, spacing and new line are still off

> +tmp = ( 5*low[i*low_stride] + 4*low[(i-1)*low_stride] - 
> low[(i-2)*low_stride] + 4) >> 3;
> +output[(2*i+0)*out_stride] = (tmp + high[i*high_stride]) >> 1;
> +if (clip)
> +output[(2*i+0)*out_stride] = 
> av_clip_uintp2(output[(2*i+0)*out_stride], clip);
> +
> +tmp = (11*low[i*low_stride] - 4*low[(i-1)*low_stride] + 
> low[(i-2)*low_stride] + 4) >> 3;
> +output[(2*i+1)*out_stride] = (tmp - high[i*high_stride]) >> 1;
> +if (clip)
> +output[(2*i+1)*out_stride] = 
> av_clip_uintp2(output[(2*i+1)*out_stride], clip);
> +}
> +else {
> +tmp = (low[(i-1)*low_stride] - low[(i+1)*low_stride] + 4) >> 3;
> +output[(2*i+0)*out_stride] = (tmp + low[i*low_stride] + 
> high[i*high_stride]) >> 1;
> +if (clip)
> +output[(2*i+0)*out_stride] = 
> av_clip_uintp2(output[(2*i+0)*out_stride], clip);
> +
> +tmp = (low[(i+1)*low_stride] - low[(i-1)*low_stride] + 4) >> 3;
> +output[(2*i+1)*out_stride] = (tmp + low[i*low_stride] - 
> high[i*high_stride]) >> 1;
> +if (clip)
> +output[(2*i+1)*out_stride] = 
> av_clip_uintp2(output[(2*i+1)*out_stride], clip);
> +}
> +}
> +}

+1 on the dsp suggestion

> +}
> +
> +static int alloc_buffers(AVCodecContext *avctx)
> +{
> +CFHDContext *s = avctx->priv_data;
> +int i, j, k;
> +
> +avctx->width = s->coded_width;
> +avctx->height = s->coded_height;

I think ff_set_dimensions is the function you're looking for (make
sure to check its return value)

> +avcodec_get_chroma_sub_sample(avctx->pix_fmt, &s->chroma_x_shift, 
> &s->chroma_y_shift);

this again? :)

> +for (i = 0; i < 3; i++) {
> +int width = i ? avctx->width >> s->chroma_x_shift : avctx->width;
> +int height = i ? avctx->height >> s->chroma_y_shift : avctx->height;

could these be candidates for AV_CEIL_RSHIFT?

> +int stride = FFALIGN(width / 8, 8) * 8;
> +int w8, h8, w4, h4, w2, h2;
> +height = FFALIGN(height / 8, 2) * 8;
> +s->plane[i].width = width;
> +s->plane[i].height = height;
> +s->plane[i].stride = stride;
> +
> +w8 = FFALIGN(s->plane[i].width / 8, 8);
> +h8 = FFALIGN(s->plane[i].height / 8, 2);
> +w4 = w8 * 2;
> +h4 = h8 * 2;
> +w2 = w4 * 2;
> +h2 = h4 * 2;
> +
> +s->plane[i].idwt_buf = av_malloc(height * stride * 
> sizeof(*s->plane[i].idwt_buf));
> +s->plane[i].idwt_tmp = av_malloc(height * stride * 
> sizeof(*s->plane[i].idwt_tmp));
> +if (!s->plane[i].idwt_buf || !s->plane[i].idwt_tmp) {
> +return AVERROR(ENOMEM);

need to av_freep both since you don't know which one failed

> +}

> +av_log(avctx, AV_LOG_DEBUG, "Start subband coef

Re: [FFmpeg-devel] [libav-devel] [PATCH 1/2] avcodec: Add Cineform HD Decoder

[Vittorio Giovara]

On Sun, Jan 10, 2016 at 1:28 AM, Kieran Kunhya  wrote:
> ---
>  libavcodec/Makefile |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h|   1 +
>  libavcodec/cfhd.c   | 565 
> 
>  libavcodec/cfhd.h   |  99 +
>  libavcodec/cfhddata.c   | 470 
>  libavcodec/codec_desc.c |   6 +
>  7 files changed, 1143 insertions(+)
>  create mode 100644 libavcodec/cfhd.c
>  create mode 100644 libavcodec/cfhd.h
>  create mode 100644 libavcodec/cfhddata.c

> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
> new file mode 100644
> index 000..dc36889
> --- /dev/null
> +++ b/libavcodec/cfhd.c
> +for (i = 0; i < len; i++) {
> +if( i == 0 )
> +{
> +tmp = (11*low[0*low_stride] - 4*low[1*low_stride] + 
> low[2*low_stride] + 4) >> 3;
> +output[(2*i+0)*out_stride] = (tmp + high[0*high_stride]) >> 1;
> +tmp = ( 5*low[0*low_stride] + 4*low[1*low_stride] - 
> low[2*low_stride] + 4) >> 3;
> +output[(2*i+1)*out_stride] = (tmp - high[0*high_stride]) >> 1;
> +}
> +else if( i == len-1 )
> +{
> +tmp = ( 5*low[i*low_stride] + 4*low[(i-1)*low_stride] - 
> low[(i-2)*low_stride] + 4) >> 3;
> +output[(2*i+0)*out_stride] = (tmp + high[i*high_stride]) >> 1;
> +tmp = (11*low[i*low_stride] - 4*low[(i-1)*low_stride] + 
> low[(i-2)*low_stride] + 4) >> 3;
> +output[(2*i+1)*out_stride] = (tmp - high[i*high_stride]) >> 1;
> +}
> +else
> +{
> +tmp = (low[(i-1)*low_stride] - low[(i+1)*low_stride] + 4) >> 3;
> +output[(2*i+0)*out_stride] = (tmp + low[i*low_stride] + 
> high[i*high_stride]) >> 1;
> +tmp = (low[(i+1)*low_stride] - low[(i-1)*low_stride] + 4) >> 3;
> +output[(2*i+1)*out_stride] = (tmp + low[i*low_stride] - 
> high[i*high_stride]) >> 1;
> +}
> +}
> +}

this formatting will make Diego cry blood

> +static void horiz_filter(int16_t *output, int16_t *low, int16_t *high, int 
> width)
> +{
> +filter(output, 1, low, 1, high, 1, width);
> +}
> +
> +static void vert_filter(int16_t *output, int out_stride, int16_t *low, int 
> low_stride,
> +int16_t *high, int high_stride, int len)
> +{
> +filter(output, out_stride, low, low_stride, high, high_stride, len);
> +}

these might be very good av_always_inline candidates

> +static int cfhd_decode(AVCodecContext *avctx, void *data, int *got_frame,
> +   AVPacket *avpkt)
> +{
> +CFHDContext *s = avctx->priv_data;
> +GetByteContext gb;
> +AVFrame *pic = data;
> +int ret = 0, i, j;
> +int16_t *plane[3] = {NULL};
> +int16_t *tmp[3] = {NULL};
> +int16_t *subband[3][10] = {{0}};
> +int16_t *l_h[3][8];
> +int16_t *coeff_data;
> +
> +avcodec_get_chroma_sub_sample(avctx->pix_fmt, &s->chroma_x_shift, 
> &s->chroma_y_shift);

this is a deprecated function, looks like you have to use
av_pix_fmt_get_chroma_sub_sample and check its return value

> +for (i = 0; i < 3; i++) {
> +int width = i ? avctx->width >> s->chroma_x_shift : avctx->width;
> +int height = i ? avctx->height >> s->chroma_y_shift : avctx->height;
> +int stride = FFALIGN(width / 8, 8) * 8;
> +int w8, h8, w4, h4, w2, h2;
> +height = FFALIGN(height / 8, 2) * 8;
> +s->plane[i].width = width;
> +s->plane[i].height = height;
> +s->plane[i].stride = stride;
> +
> +w8 = FFALIGN(s->plane[i].width / 8, 8);
> +h8 = FFALIGN(s->plane[i].height / 8, 2);
> +w4 = w8 * 2;
> +h4 = h8 * 2;
> +w2 = w4 * 2;
> +h2 = h4 * 2;
> +
> +plane[i] = av_malloc(height * stride * sizeof(*plane[i]));
> +tmp[i]   = av_malloc(height * stride * sizeof(*tmp[i]));
> +if (!plane[i] || !tmp[i]) {
> +ret = AVERROR(ENOMEM);
> +goto end;
> +}
> +
> +subband[i][0] = plane[i];
> +subband[i][1] = plane[i] + 2 * w8 * h8;
> +subband[i][2] = plane[i] + 1 * w8 * h8;
> +subband[i][3] = plane[i] + 3 * w8 * h8;
> +subband[i][4] = plane[i] + 2 * w4 * h4;
> +subband[i][5] = plane[i] + 1 * w4 * h4;
> +subband[i][6] = plane[i] + 3 * w4 * h4;
> +subband[i][7] = plane[i] + 2 * w2 * h2;
> +subband[i][8] = plane[i] + 1 * w2 * h2;
> +subband[i][9] = plane[i] + 3 * w2 * h2;
> +
> +l_h[i][0] = tmp[i];
> +l_h[i][1] = tmp[i] + 2 * w8 * h8;
> +//l_h[i][2] = ll2;
> +l_h[i][3] = tmp[i];
> +l_h[i][4] = tmp[i] + 2 * w4 * h4;
> +//l_h[i][5] = ll1;
> +l_h[i][6] = tmp[i];
> +l_h[i][7] = tmp[i] + 2 * w2 * h2;
> +}
> +
> +init_frame_defaults(s);
> +
> +if ((ret = ff_get_buffer(avctx, pic, 0)) < 0)
> +return ret;

you are leaking plane and tmp in case of error, this 

Re: [FFmpeg-devel] [libav-devel] [PATCH] Cineform HD Decoder:

[Vittorio Giovara]

On Mon, Jan 4, 2016 at 1:04 AM, Kieran Kunhya  wrote:
> Decodes YUV422P10 samples in AVI and MOV (Gopro Studio)
> Older files with more subbands, skips, Bayer not supported
> ---

> +static av_cold int cfhd_init_decoder(AVCodecContext *avctx)
> +{
> +CFHDContext *s = avctx->priv_data;
> +
> +ff_cfhd_init_vlcs(s);
> +
> +avctx->pix_fmt = AV_PIX_FMT_YUV422P10;
> +avctx->bits_per_raw_sample = 16;
> +s->avctx   = avctx;
> +
> +return 0;

here could be return ff_cfhd_init_vlcs(s); (see below)

> +static av_cold int cfhd_close_decoder(AVCodecContext *avctx)
> +{
> +CFHDContext *s = avctx->priv_data;
> +
> +ff_free_vlc(&s->vlc_9);
> +ff_free_vlc(&s->vlc_18);
> +
> +return 0;
> +}

> +AVCodec ff_cfhd_decoder = {
> +.name   = "cfhdvideo",
> +.long_name  = NULL_IF_CONFIG_SMALL("cfhd video"),
> +.type   = AVMEDIA_TYPE_VIDEO,
> +.id = AV_CODEC_ID_CFHD,
> +.priv_data_size = sizeof(CFHDContext),
> +.init   = cfhd_init_decoder,
> +.close  = cfhd_close_decoder,
> +.decode = cfhd_decode,
> +.capabilities   = AV_CODEC_CAP_EXPERIMENTAL,

also DR1 I believe

> +};

> +av_cold int ff_cfhd_init_vlcs(CFHDContext *s)
> +{
> +init_vlc(&s->vlc_9, VLC_BITS, j, new_cfhd_vlc_len,
> + 1, 1, new_cfhd_vlc_bits, 4, 4, 0);

> +init_vlc(&s->vlc_18, VLC_BITS, j, new_cfhd_vlc_len,
> + 1, 1, new_cfhd_vlc_bits, 4, 4, 0);
> +assert(s->vlc_18.table_size == 4572);

my only suggestion here is that since this isn't using static vars you
should add FF_CODEC_CAP_INIT_THREADSAFE to the internal capabilities,
and since allocating memory you should check the init_vlc return
value, return it and add the proper FF_CODEC_CAP_INIT_CLEANUP to do
the cleanup work for you

if you feel like it you could use avpriv_report_missing_feature
instead of av_log where you return AVERROR_PATCHWELCOME
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Adding frame side data about green metadata

[Vittorio Giovara]

On Wed, Dec 23, 2015 at 12:28 PM, Nicolas Derouineau
 wrote:
> Hello,
> Please find here enclosed a patch adding frame side data information about 
> green metadata.

The logic is ok, but the introduction of av_greenmetadata_set is a
no-go for me. This function is exposed to every user (with a non-fixed
data type) and its only purpose is to set the values in an array.
There is no need for that: if you believe that there are going to be
many codecs needing this functionality, just add a private function in
a separate file within libavcodec, otherwise you may initialize the
array directly in the sei_green_metadata_present check in h264.c
without any function call at all.

As a minor nit, please keep the code style consistent, with spaces
around '=', 'if's and logic operators.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] GreenMetadata complexity metric access through high level API

[Vittorio Giovara]

On Mon, Nov 30, 2015 at 8:21 AM, Nicolas Derouineau
 wrote:
> Hello
>
>>you should look at avframesidedata
>
> Actually the issue is that the green metadatadata are concerning the upcoming 
> frame (They are used to predict the upcoming complexity).
>
> The avframesidedata are concerning a frame which is already decoded (so it's 
> not useful in the case of greenmetadata).
>
> We made a proof of concept using a home made parsing function (replacing 
> av_read_frame() by a custom one), but we are thinking about a more portable 
> solution.

I am definitely not familiar with what the green metadata is or does,
but from the sound of it I still think it is suited for
AVFrameSideData: it's data which is not essential for decoding, it's
frame bound, and applications outside lavc might need to act
differently depending on its contents.

Even if this green metadata is used within libavcodec and concerns the
upcoming frame, there is still a way to make it work: I believe that
frame side data in ffmpeg are ref counted (meaning you can just add a
reference to the side data rather than allocating/copying data), put
it in a queue of green metadata and apply it when the upcoming frame
is decoded or it's about to be decoded. You would have to be careful
only in frame reordering, but the rest should be pretty simple.

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

Re: [FFmpeg-devel] [libav-devel] [PATCH] hqx: correct type and size check of info_offset

[Vittorio Giovara]

On Sun, Nov 15, 2015 at 10:50 AM, Andreas Cadhalpun
 wrote:
> It is used as size argument of ff_canopus_parse_info_tag, which uses it
> as size argument to bytestream2_init, which only supports sizes up to
> INT_MAX.
> Changing it's type to unsigned simplifies the check.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/hqx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> index 8060c7a..138d960 100644
> --- a/libavcodec/hqx.c
> +++ b/libavcodec/hqx.c
> @@ -417,8 +417,8 @@ static int hqx_decode_frame(AVCodecContext *avctx, void 
> *data,
>
>  info_tag= AV_RL32(src);
>  if (info_tag == MKTAG('I', 'N', 'F', 'O')) {
> -int info_offset = AV_RL32(src + 4);
> -if (info_offset > UINT32_MAX - 8 || info_offset + 8 > avpkt->size) {
> +unsigned info_offset = AV_RL32(src + 4);
> +if (info_offset > INT_MAX || info_offset + 8 > avpkt->size) {
>  av_log(avctx, AV_LOG_ERROR,
> "Invalid INFO header offset: 0x%08"PRIX32" is too 
> large.\n",
> info_offset);
> --
> 2.6.2

lgtm, thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Vittorio Giovara]

On Fri, Nov 13, 2015 at 10:01 PM, Andreas Cadhalpun
 wrote:
> On 13.11.2015 02:08, Vittorio Giovara wrote:
>> oh I see, that can happen for a special crafted file, DDPF_FOURCC has
>> been introduced recently while DDPF_PALETTE has been removed, so a
>> normal file should not have both set.
>
> OK, that makes sense.
>
>> Because of that, and how rare palette dds are, I think it's quite safe
>> to unset ctx->paletted if ctx->compressed is set, and be done with it.
>
> Patch doing that is attached.

lgtm, thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 8:29 PM, Andreas Cadhalpun
 wrote:
> On 11.11.2015 12:28, Vittorio Giovara wrote:
>> On Wed, Nov 11, 2015 at 1:16 AM, Andreas Cadhalpun
>>  wrote:
>>> Otherwise it causes a NULL pointer dereference of frame->data[1].
>>>
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  libavcodec/dds.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
>>> index c918cf0..fe36709 100644
>>> --- a/libavcodec/dds.c
>>> +++ b/libavcodec/dds.c
>>> @@ -662,6 +662,11 @@ static int dds_decode(AVCodecContext *avctx, void 
>>> *data,
>>>
>>>  if (ctx->paletted) {
>>>  int i;
>>> +if (!frame->data[1]) {
>>> +av_log(avctx, AV_LOG_ERROR,
>>> +   "Palette frame buffer is not allocated.\n");
>>> +return AVERROR_INVALIDDATA;
>>> +}
>>>  /* Use the first 1024 bytes as palette, then copy the rest. */
>>>  bytestream2_get_buffer(gbc, frame->data[1], 256 * 4);
>>>  for (i = 0; i < 256; i++)
>>
>> how can this happen?
>
> That's best described with code:
> if (!ctx->compressed && ctx->paletted &&
> !(av_pix_fmt_desc_get(avctx->pix_fmt)->flags & (AV_PIX_FMT_FLAG_PAL | 
> AV_PIX_FMT_FLAG_PSEUDOPAL)))
>
> Attached is a patch using this expression to check for the problem.

Sorry, I wasn't clear with my question. What I meant is that in
video_get_buffer() (frame.c) explicitly allocates data[1] when any
pixel format with AV_PIX_FMT_FLAG_PAL is requested, and in dds when
(ctx->paletted) is set, then AV_PIX_FMT_PAL8 is the pixel format
chosen every time. So unless I'm missing something data[1] is always
allocated, no?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 1/4] dds: validate source buffer size before copying

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 1:14 AM, Andreas Cadhalpun
 wrote:
> If it is too small av_image_copy_plane segfaults.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/dds.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
> index a604d56..324e665 100644
> --- a/libavcodec/dds.c
> +++ b/libavcodec/dds.c
> @@ -666,6 +666,12 @@ static int dds_decode(AVCodecContext *avctx, void *data,
>  frame->palette_has_changed = 1;
>  }
>
> +if (bytestream2_get_bytes_left(gbc) < frame->height * linesize) {
> +av_log(avctx, AV_LOG_ERROR, "Buffer is too small (%d < %d).\n",
> +   bytestream2_get_bytes_left(gbc), frame->height * 
> linesize);
> +return AVERROR_INVALIDDATA;
> +}
> +
>  av_image_copy_plane(frame->data[0], frame->linesize[0],
>  gbc->buffer, linesize,
>  linesize, frame->height);
> --
> 2.6.2

Same thought of 2/4 but patch should be ok.
Thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 2/4] dds: validate compressed source buffer size

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 1:15 AM, Andreas Cadhalpun
 wrote:
> A too small buffer will cause segfaults somewhere below
> decompress_texture_thread.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/dds.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
> index 324e665..c918cf0 100644
> --- a/libavcodec/dds.c
> +++ b/libavcodec/dds.c
> @@ -642,9 +642,18 @@ static int dds_decode(AVCodecContext *avctx, void *data,
>  return ret;
>
>  if (ctx->compressed) {
> +int size = (avctx->coded_height / TEXTURE_BLOCK_H) *
> +   (avctx->coded_width / TEXTURE_BLOCK_W) * ctx->tex_ratio;
>  ctx->slice_count = av_clip(avctx->thread_count, 1,
> avctx->coded_height / TEXTURE_BLOCK_H);
>
> +if (bytestream2_get_bytes_left(gbc) < size) {
> +av_log(avctx, AV_LOG_ERROR,
> +   "Compressed Buffer is too small (%d < %d).\n",
> +   bytestream2_get_bytes_left(gbc), size);
> +return AVERROR_INVALIDDATA;
> +}
> +
>  /* Use the decompress function on the texture, one block per thread. 
> */
>  ctx->tex_data = gbc->buffer;
>  avctx->execute2(avctx, decompress_texture_thread, frame, NULL, 
> ctx->slice_count);
> --

Not sure if we should check this before the ff_get_buffer to avoid an
allocation in case of error, but I think the patch is correct.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 1:16 AM, Andreas Cadhalpun
 wrote:
> Otherwise it causes a NULL pointer dereference of frame->data[1].
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/dds.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
> index c918cf0..fe36709 100644
> --- a/libavcodec/dds.c
> +++ b/libavcodec/dds.c
> @@ -662,6 +662,11 @@ static int dds_decode(AVCodecContext *avctx, void *data,
>
>  if (ctx->paletted) {
>  int i;
> +if (!frame->data[1]) {
> +av_log(avctx, AV_LOG_ERROR,
> +   "Palette frame buffer is not allocated.\n");
> +return AVERROR_INVALIDDATA;
> +}
>  /* Use the first 1024 bytes as palette, then copy the rest. */
>  bytestream2_get_buffer(gbc, frame->data[1], 256 * 4);
>  for (i = 0; i < 256; i++)

how can this happen?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 4/4] dds: add missing newline to log messages

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 1:16 AM, Andreas Cadhalpun
 wrote:
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/dds.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
> index fe36709..4d68b33 100644
> --- a/libavcodec/dds.c
> +++ b/libavcodec/dds.c
> @@ -599,14 +599,14 @@ static int dds_decode(AVCodecContext *avctx, void *data,
>  bytestream2_init(gbc, avpkt->data, avpkt->size);
>
>  if (bytestream2_get_bytes_left(gbc) < 128) {
> -av_log(avctx, AV_LOG_ERROR, "Frame is too small (%d).",
> +av_log(avctx, AV_LOG_ERROR, "Frame is too small (%d).\n",
> bytestream2_get_bytes_left(gbc));
>  return AVERROR_INVALIDDATA;
>  }
>
>  if (bytestream2_get_le32(gbc) != MKTAG('D', 'D', 'S', ' ') ||
>  bytestream2_get_le32(gbc) != 124) { // header size
> -av_log(avctx, AV_LOG_ERROR, "Invalid DDS header.");
> +av_log(avctx, AV_LOG_ERROR, "Invalid DDS header.\n");
>  return AVERROR_INVALIDDATA;
>  }
>
> --
> 2.6.2

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

Re: [FFmpeg-devel] [libav-devel] [PATCH 20/20] Bump major versions of all libraries

[Vittorio Giovara]

On Wed, Aug 26, 2015 at 12:47 AM, Andreas Cadhalpun
 wrote:
> On 25.08.2015 11:03, Vittorio Giovara wrote:
>> There is, consensus does not need to be unanimous, and so far only you
>> have been expressing concerns (multiple times).
>
> That's not true.
> wm4, James and indirectly Ronald also had concerns.

Actually they were advocating to remove all the deprecated features,
and they mostly have been in favour of the proposed deprecations to my
knowledge.

> And there have been more on ffmpeg-devel.

And this is relevant how? ffmpeg is not bound to the deprecations or
removals libav does, and vice versa.

>> if you're talking about the patch set you sent it, I don't believe
>> it's a good idea adding yet-another-doc-file to the tree,
>
> Having this in the tree is important, because then one can make
> sure that whenever something gets deprecated the necessary
> documentation for API users gets added.

Not really, as humans are fallible, it is very easy to forget to
update a documentation file. Updating a checked-in is also much more
convoluted than a wiki page. See how many codecs are missing from
doc/general.texi as another example. A new file for this purpose is
just another maintenance burden which will not accomplish anything.

>> it mostly duplicates APIChanges
>
> No, APIchanges lacks code examples and it contains too much unrelated
> stuff to be useful as porting guide.

What unrelated stuff? Why are you complaining about this now?
APIChanges is a starting guide which contains the minimum changes one
should be aware of when updating the code, documentation can be found
elsewhere (on a wiki).

>> and its contents are much better off on a wiki,
>
> That has been tried and it didn't work that well. The current wiki pages
> don't even mention many API deprecations/removals.

Are you sure? https://wiki.libav.org/Migration/10 looks pretty
complete, feel free to amend it if you want to add more. Bonus, you
don't need git, mails or reviews for that since it's a just wiki page
everyone can edit.

>> where it can be quickly updated and edited. Your efforts are indeed
>> commendable and I look forward to seeing the wiki page I linked filled
>> with documentation.
>
> You can copy the API-porting-guide to the wiki, whenever a release is
> made.

Why? API-porting-guide as you envisioned should not exist in the first
place, and its contents should be written directly to the wiki. This
will encourage people to contribute rather than having a single guy do
all the work. Also, filling that wiki rather than repeating over and
over the same arguments here would be much more beneficial for
everyone.

>>>> If no objections I'll push this set in the following days.
>>>
>>> Can you explain why you believe it makes any kind of sense to remove
>>> widely used APIs like FF_API_PIX_FMT/FF_API_AVCODEC_FRAME, while
>>> keeping completely useless ones like FF_API_MISSING_SAMPLE?
> And anyway, doing these removals strictly according to the calendar
> doesn't make sense.

Nor does "being widely used" or no deprecation would ever take place.
We've been over this multiple times and apparently you refuse to
acknowledge any arguments many developers made. I think we'll just
have to agree to disagree if you insist on this point.

> The reason for keeping deprecated APIs is, as you say, that
> "downstream users need time to update their applications".
> For widely used APIs this naturally takes longer than for rarely
> used ones.

No, the time needed to update an API depends on how convoluted the API
change is.
For example in porting get_buffer -> get_buffer2 you need to know what
you are doing whereas for avcodec_alloc_frame you need to change a
couple of functions calls. Once again "being widely used" is a moot
argument here.

>> Jokes aside, missing_sample can go as well if you insist,
>
> Yes, I do insist that the decision which APIs get removed now and which
> are kept makes some kind of sense.
> Apropos, delaying FF_API_NOCONST_GET_NAME also doesn't make any sense,
> because API users can't adapt to that before it happened.

I have no idea what you are talking about here. Feel free to send a
patch if you want additional removals to take place while this window
is open.

>> while for
>> the other two you mention do you have any other argument than being
>> "widely used"?
>
> That's a way better argument than your argument for FF_API_MISSING_SAMPLE.

You proposed to remove this, are you withdrawing your request? I am
not sure I follow you any more.

> What's your argument for keeping the ones you suggested?
> In particular I'm interested in explanations for the following, unused APIs:

Re: [FFmpeg-devel] [libav-devel] [PATCH 20/20] Bump major versions of all libraries

[Vittorio Giovara]

On Mon, Aug 24, 2015 at 11:43 PM, Andreas Cadhalpun
 wrote:
> On 24.08.2015 13:44, Vittorio Giovara wrote:
>> On Tue, Jul 28, 2015 at 6:54 PM, Luca Barbato  wrote:
>>> On 28/07/15 15:41, Vittorio Giovara wrote:
>>>> On Tue, Jul 28, 2015 at 2:40 PM, Luca Barbato  wrote:
>>>>> On 28/07/15 15:30, Vittorio Giovara wrote:
>> I believe to see general consensus towards applying the set as is.
>
> There is no consensus for that.

There is, consensus does not need to be unanimous, and so far only you
have been expressing concerns (multiple times).

>> I've added a skeleton to the wiki
>> (https://wiki.libav.org/Migration/12) so that we can properly document
>> the necessary changes before we release. Any help with that is of
>> course welcome.
>
> I have a work-in-progress API-porting-guide.

if you're talking about the patch set you sent it, I don't believe
it's a good idea adding yet-another-doc-file to the tree, it mostly
duplicates APIChanges and its contents are much better off on a wiki,
where it can be quickly updated and edited. Your efforts are indeed
commendable and I look forward to seeing the wiki page I linked filled
with documentation.

>> If no objections I'll push this set in the following days.
>
> Can you explain why you believe it makes any kind of sense to remove
> widely used APIs like FF_API_PIX_FMT/FF_API_AVCODEC_FRAME, while
> keeping completely useless ones like FF_API_MISSING_SAMPLE?

I was afraid someone would point out it's less than two years old.
Jokes aside, missing_sample can go as well if you insist, while for
the other two you mention do you have any other argument than being
"widely used"? I believe we went over and over explaining why it's a
good a idea to remove those, not sure how beneficial it is to iterate
once again.

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

Re: [FFmpeg-devel] [libav-devel] [PATCH 4/6] avcodec: delay removal of avcodec_*_frame

[Vittorio Giovara]

On Sat, Aug 8, 2015 at 1:37 PM, Andreas Cadhalpun
 wrote:
> They are still widely used and have been deprecated for less than two years.

are we talking about ad0c9f2? if so, it's dated 2012-10-08, so almost
three years.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 0/20] removal of deprecated features

[Vittorio Giovara]

On Thu, Jul 30, 2015 at 6:10 PM, Andreas Cadhalpun
 wrote:
> Removing these APIs causes compile failures, which are more severe than
> occasional runtime failures. It means people have to use old versions of
> the libav* libraries.

I disagree as well, it's true that noone likes code failing to
compile, but a runtime misbehaviour is several orders of magnitude
worse. Also, in my opinion, if maintainers or the community don't
upgrade their code (or code they care about) when APIs change, it's
not a good sign of a healthy, engaged environment, where code can be
left bitrotting.

>> I actually did fix a huge number of packages during the last two API
>> breaks. But it's not really reasonable to expect me to do it all the
>> time.
>
> Certainly. But someone has to do the work, if you want to remove widely
> used APIs.

"widely used APIs" that were marked deprecated two years ago. That
someone is the downstream project maintainer.

Exactly like there is a "promise" that API won't be broken (unless
necessary) between minor releases, by using libav APIs you "agree"
that there can be breakage between major, and for good reasons too.
The "announcement" is in the form of warnings when you try to use the
API. Not removing deprecated APIs when the time is due would mean that
no deprecation would be taken seriously, thus neutering the whole
concept of deprecating code.

Also please kindly stop saying "you should do this" or "you should do
that", noone is asking you to do anything, so don't try to impose
additional expectations on libav maintainers. Finally there is still
time before distributions pick up candidates for new releases, both of
the library and of the downstream project, so I don't really see any
use of complaining now (two years *after* the deprecation was
announced).

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

Re: [FFmpeg-devel] GreenMetadata complexity metric access through high level API

[Vittorio Giovara]

On Wed, Jul 1, 2015 at 11:08 AM, Nicolas Derouineau
 wrote:
> Hello,
>
> H264 GreenMetadata parsing is now supported in ffmpeg, but the informations 
> are stored in the h264context, which is not available at a higher level.
>
> Is it possible to add a new field named "upcoming complexity metrics", which 
> should be codec agnostic, either in the avcodeccontext struct or in the 
> AVFrame struct ?

you should look at avframesidedata
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Patch to parse H264 SEI Green Metadata

[Vittorio Giovara]

On Thu, Jun 25, 2015 at 5:30 PM, Michael Niedermayer
 wrote:
> On Thu, Jun 25, 2015 at 11:13:53AM +, Nicolas Derouineau wrote:
>> Hello,
>> Please find here enclosed a patch enabling h264 Green Metada SEI parsing for 
>> FFMPEG. You'll be able to find reference bitstreams containing the metadata 
>> at the following adress:
>>
>> ftp-public-greenvideo.insa-rennes.fr
>>
>>
>> The Nal SEI syntax is the same as the one used in the last JM release (19.0).
>>
>>
>> Best Regards,
>>
>>
>>
>> Nicolas DEROUINEAU
>> Research Engineer
>> VITEC
>>
>> T.  +33 1 46 73 06 06
>> E.  nicolas.derouin...@vitec.com
>> W. www.vitec.com
>
>>  h264.h |   20 ++
>>  h264_sei.c |   67 
>> +
>>  2 files changed, 87 insertions(+)
>> feb39a55dd6afbaf341df765eafc02266c00a588  
>> 0002-Enabling-GreenMetadata-SEI-parsing-for-H264-decoder.patch
>> From 60903bff6429182c84dc5daef0d26695d3f71861 Mon Sep 17 00:00:00 2001
>> From: Nicolas DEROUINEAU 
>> Date: Thu, 25 Jun 2015 13:02:39 +0200
>> Subject: [PATCH 2/2] Enabling GreenMetadata SEI parsing for H264 decoder
>>
>> ---
>>  libavcodec/h264.h | 20 +++
>>  libavcodec/h264_sei.c | 67 
>> +++
>>  2 files changed, 87 insertions(+)
>>
>> diff --git a/libavcodec/h264.h b/libavcodec/h264.h
>> index 548510d..0324dc1 100644
>> --- a/libavcodec/h264.h
>> +++ b/libavcodec/h264.h
>> @@ -137,6 +137,7 @@ typedef enum {
>>  SEI_TYPE_RECOVERY_POINT = 6,   ///< recovery point (frame # to 
>> decoder sync)
>>  SEI_TYPE_FRAME_PACKING  = 45,  ///< frame packing arrangement
>>  SEI_TYPE_DISPLAY_ORIENTATION= 47,  ///< display orientation
>> +SEI_TYPE_GREEN_METADATA  = 56  ///< GreenMPEG information
>>  } SEI_Type;
>>
>>  /**
>> @@ -268,6 +269,22 @@ typedef struct FPA {
>>  } FPA;
>>
>>  /**
>> + * Green MetaData Information Type
>> + */
>> +typedef struct GreenMetaData {
>> +unsigned char  green_metadata_type;
>> +unsigned char  period_type;
>> +unsigned short num_seconds;
>> +unsigned short num_pictures;

uint16_t and uint8_t would probably be more appropriate here

>> +unsigned char percent_non_zero_macroblocks;
>> +unsigned char percent_intra_coded_macroblocks;
>> +unsigned char percent_six_tap_filtering;
>> +unsigned char percent_alpha_point_deblocking_instance;
>> +unsigned char xsd_metric_type;
>> +unsigned short xsd_metric_value;
>> +} GreenMetaData;
>> +
>> +/**
>>   * Memory management control operation opcode.
>>   */
>>  typedef enum MMCOOpcode {
>> @@ -804,6 +821,9 @@ typedef struct H264Context {
>>  /* Motion Estimation */
>>  qpel_mc_func (*qpel_put)[16];
>>  qpel_mc_func (*qpel_avg)[16];
>> +
>> +/*Green Metadata */
>> +GreenMetaData sei_GreenMetaData;

This is mostly a nit, but could you maybe avoid using CamelCase naming
in function and variable names?
eg GreenMetaData sei_green_metadata would reflect more the rest of the code.

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

Re: [FFmpeg-devel] [PATCH] libavformat: Add H264 API test

[Vittorio Giovara]

On Mon, Jun 22, 2015 at 2:50 PM, Derek Buitenhuis
 wrote:
> On 6/22/2015 2:46 PM, Vittorio Giovara wrote:
>> afaik in POSIX any non zero value is to be considered an error, also
>> because value ranges on an unsigned byte.
>
> -1 ends up greater than 128, which is reserved by POSIX for system
> signal info (SIGKILL and pals).

'k
the return value of video_decode_example() should still be checked
though (and maybe use the EXIT_* macros then?)
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat: Add H264 API test

[Vittorio Giovara]

On Mon, Jun 22, 2015 at 1:40 PM, Derek Buitenhuis
 wrote:
> On 6/22/2015 1:32 PM, Vittorio Giovara wrote:
>> video_decode_example can return -1 on error, and this is lost, so
>> you'd better do "return video_decode_example(argv[1]);" to return the
>> value to the caller. Also sometimes you exit(1) and sometimes you
>> return -1, maybe you could go with only one of them.
>
> You shouldn't return -1 as a process exit code. This is a reserved value
> for proc return codes in POSIX.

afaik in POSIX any non zero value is to be considered an error, also
because value ranges on an unsigned byte.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat: Add H264 API test

[Vittorio Giovara]

On Mon, Jun 22, 2015 at 10:50 AM, Ludmila Glinskih  wrote:
> +static int video_decode_example(const char *input_filename)
> +{
> +AVCodec *codec = NULL;
> +AVCodecContext *origin_ctx = NULL, *ctx= NULL;
> +AVFrame *fr = NULL;
> +uint8_t *byte_buffer = NULL;
> +AVPacket pkt;
> +AVFormatContext *fmt_ctx = NULL;
> +int number_of_written_bytes;
> +int video_stream;
> +int get_frame = 0;
> +int byte_buffer_size;
> +int i = 0;
> +
> +
> +if (avformat_open_input(&fmt_ctx, input_filename, NULL, NULL) < 0) {
> +fprintf(stderr, "Could not open source file %s\n", input_filename);
> +exit(1);
> +}
> +
> +if (avformat_find_stream_info(fmt_ctx, NULL) < 0) {
> +fprintf(stderr, "Could not find stream information\n");
> +exit(1);
> +}
> +
> +video_stream = -1;
> +for (i = 0; i < fmt_ctx->nb_streams; i++) {
> +if (fmt_ctx->streams[i]->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
> +video_stream = i;
> +break;
> +}
> +}
> +
> +origin_ctx = fmt_ctx->streams[video_stream]->codec;
> +
> +codec = avcodec_find_decoder(origin_ctx->codec_id);
> +if (codec == NULL) {
> +return -1;
> +}
> +ctx = avcodec_alloc_context3(codec);
> +if (ctx == NULL) {
> +return -1;
> +}
> +
> +if (avcodec_copy_context(ctx, origin_ctx)) {
> +return -1;
> +}
> +
> +if (avcodec_open2(ctx, codec, NULL) < 0) {
> +return -1;
> +}
> +
> +fr = av_frame_alloc();
> +if (fr == NULL) {
> +return -1;
> +}
> +
> +byte_buffer_size = av_image_get_buffer_size(ctx->pix_fmt, ctx->width, 
> ctx->height, 16);
> +byte_buffer = av_malloc(byte_buffer_size);
> +
> +printf("#tb %d: %d/%d\n", video_stream, 
> fmt_ctx->streams[video_stream]->time_base.num, 
> fmt_ctx->streams[video_stream]->time_base.den);
> +i = 0;
> +av_init_packet(&pkt);
> +while (av_read_frame(fmt_ctx, &pkt) >= 0) {
> +if (pkt.stream_index == video_stream) {
> +get_frame = 0;
> +if (pkt.pts == AV_NOPTS_VALUE)
> +pkt.pts = pkt.dts = i;
> +avcodec_decode_video2(ctx, fr, &get_frame, &pkt);
> +if (get_frame) {
> +number_of_written_bytes = 
> av_image_copy_to_buffer(byte_buffer, byte_buffer_size,
> +(const uint8_t* const *)fr->data, 
> (const int*) fr->linesize,
> +ctx->pix_fmt, ctx->width, 
> ctx->height, 1);
> +printf("%d, %10"PRId64", %10"PRId64", %8d, %8d, 
> 0x%08"PRIx32"\n", video_stream,
> +fr->pkt_pts, fr->pkt_dts, fr->pkt_duration,
> +number_of_written_bytes, av_adler32_update(0, (const 
> uint8_t*)byte_buffer, number_of_written_bytes));
> +}
> +av_free_packet(&pkt);
> +av_init_packet(&pkt);
> +}
> +i++;
> +}
> +pkt.data = NULL;
> +pkt.size = 0;
> +if (pkt.pts == AV_NOPTS_VALUE)
> +pkt.pts = pkt.dts = i;
> +int flag = 0;
> +while (!flag) {
> +if (pkt.stream_index != video_stream)
> +break;
> +get_frame = 0;
> +if (avcodec_decode_video2(ctx, fr, &get_frame, &pkt) < 0 || 
> get_frame == 0)
> +flag = 1;
> +if (get_frame) {
> +number_of_written_bytes = av_image_copy_to_buffer(byte_buffer, 
> byte_buffer_size,
> +(const uint8_t* const *)fr->data, (const 
> int*) fr->linesize,
> +ctx->pix_fmt, ctx->width, ctx->height, 
> 1);
> +printf("%d, %10"PRId64", %10"PRId64", %8d, %8d, 
> 0x%08"PRIx32"\n", video_stream,
> +fr->pkt_pts, fr->pkt_dts, fr->pkt_duration,
> +number_of_written_bytes, av_adler32_update(0, (const 
> uint8_t*)byte_buffer, number_of_written_bytes));
> +}
> +i++;
> +}
> +av_free_packet(&pkt);
> +av_frame_free(&fr);
> +avcodec_close(ctx);
> +avformat_close_input(&fmt_ctx);
> +avcodec_free_context(&ctx);
> +av_freep(&byte_buffer);
> +return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +av_register_all();
> +video_decode_example(argv[1]);
> +return 0;
> +}

video_decode_example can return -1 on error, and this is lost, so
you'd better do "return video_decode_example(argv[1]);" to return the
value to the caller. Also sometimes you exit(1) and sometimes you
return -1, maybe you could go with only one of them.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH] h264: check frame properties for consistency before copying

[Vittorio Giovara]

On Sun, Jun 14, 2015 at 11:40 AM, Andreas Cadhalpun
 wrote:
> Also use the frame pixel format instead of the one from the codec
> context, which is more robust.

Does this fix anything in particular or is just as precaution?

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/h264_slice.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 0c0812f..0183b9c 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1552,12 +1552,14 @@ int ff_h264_decode_slice_header(H264Context *h, 
> H264SliceContext *sl)
>   * vectors.  Given we are concealing a lost frame, this probably
>   * is not noticeable by comparison, but it should be fixed. */
>  if (h->short_ref_count) {
> -if (prev) {
> +if (prev && h->short_ref[0]->f->width == prev->f->width
> +&& h->short_ref[0]->f->height == prev->f->height
> +&& h->short_ref[0]->f->format == prev->f->format) {

nit: the && should be at the end of the line (if alignment is a
concern, prev may be on its own line)
no need to resend, it will be fixed by whoever pushes this
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH] h264: update avctx width/height when flushing

[Vittorio Giovara]

On Fri, Jun 12, 2015 at 9:01 PM, Andreas Cadhalpun
 wrote:
> On 12.06.2015 21:51, Hendrik Leppkes wrote:
>> On Fri, Jun 12, 2015 at 9:23 PM, Andreas Cadhalpun
>>> I agree that it is not really elegant, but removing the assumption that
>>> the avctx width/height/pix_fmt are from the last decoded frame instead
>>> of the last returned frame from the h264 decoder is beyond me.
>>> If someone more familiar with the h264 decoder could do that, it would
>>> be great.
>>> But until then, having this workaround is better than the current state.
>>> So I've pushed this.
>>>
>>
>> This patch is super ugly and a mega hack.
>
> Maybe. ;)
>
>> Just inform consumers that only AVFrame has the actual up-to-date
>> values when a change in the bitstream happens and stop doing such
>> crap.
>
> That's not currently documented, so it would effectively be an API change.
> And since the h264 decoder is the only one misbehaving in this way,
> fixing it is the better alternative.

Not necessarily, a lot of parameters are valid only in the avframe
while in the context they might not be completely updated.
The first that comes to mind is interlaced_frame -- there are 3 places
this might (or might not) be signalled (context, frame and stream or
parser iirc) but the RightWay is to check it from the frame only.

Do you have a test case where using the API exposes some kind of problem?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] shared api for exposing a texture

[Vittorio Giovara]

Hi,
following the positive trend as of late, here is a shared discussion
on a proposed API.


There are a couple of formats that are based on texture compression,
usually called DXTn or BCn, and described here:
http://en.wikipedia.org/wiki/S3_Texture_Compression. Currently in
libavcodec only txd uses this style, but there are others I am working
on, namely Hap and DSS.

What I thought while working on them (and later found out actually
being of commercial interest) is that the texture could be potentially
left intact and rather than being decoded (or encoded) internally by
libavcodec. The user might want to skip decoding the texture
altogether and decode it him or herself, possibly exploiting gpu
acceleration.

Unfortunately these formats often employ additional compression or add
custom headers so the user can't just demux and accelerate the output
frame as is. Interested codecs could let the user choose this with a
private option.


There are a couple of strategies here.
1. Introduce a pixel format for each texture: this has the advantage
of getting appropriately-sized buffers, but in the end it would
require having a different pixel format for each variant of each
texture compression. Our users tend to dislike this option and I am
afraid this would require us of constantly adding new texture formats
when support is added.
2. Introduce a single opaque pixel format: this has the advantage of
not having to update API at every new format, but leaves users in the
dark to what the texture actually is, and requires to know what he or
she is actually doing.
3. Introduce a single opaque pixel format and a side data: as a
variant of above, the side data would contain which variant of of the
texture and would let the user know how to deal with data without
anything special.
4. Write in the normal data buffers: instead of filling in rgba
buffers with decoded data, the raw texture could be written in
data[0], and the side data would help understand how to interpret it.
This could be somewhat hacky since it's not something users would
normally expect.
5. Introduce refcounted side data: just write everything in a side
data buffer and let the user act upon it on demand. Similar to idea 3,
but without introducing new pixel formats. Could be potentially
6. Write in the 'special' data buffer : similar to what is done for
paletted formats, write the texture in data[1], so that normal users
don't have to worry about anything and special users might just access
the appropriate buffers.


Every idea has some drawbacks and some benefits, we should try to
trade off between new APIs, maintenance and actual use cases. In my
opinion 5 is interesting but probably overkill for this usecase. I
like 3 for it simplicity and ease of use.
What would other lavc devs think is the more appropriate one? What
about our API users?

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

Re: [FFmpeg-devel] [libav-devel] [PATCH] aacsbr: break infinite loop in sbr_hf_calc_npatches

[Vittorio Giovara]

On Wed, Apr 22, 2015 at 5:31 PM, Claudio Freire  wrote:
> On Wed, Apr 22, 2015 at 12:54 PM, Andreas Cadhalpun
>  wrote:
>> On 22.04.2015 17:35, Claudio Freire wrote:
>>> On Wed, Apr 22, 2015 at 10:23 AM, Andreas Cadhalpun
>>>  wrote:
 +if (k == last_k && msb == last_msb) {
 +av_log(ac->avctx, AV_LOG_ERROR, "patch construction 
 failed\n");
 +return AVERROR_INVALIDDATA;
 +}
 +last_k = k;
 +last_msb = msb;

Andreas, do you have a sample that triggers the infinite loop?

>>>
>>>
>>> I don't think the INVALIDDATA return will have the desired effect.
>>>
>>> I think you want return -1;
>>
>> This function is only called once and there the check is:
>> if (sbr_hf_calc_npatches(ac, sbr) < 0)
>> return -1;
>>
>> Thus returning AVERROR_INVALIDDATA works as well as -1.
>
> The fact that AVERROR_INVALIDDATA < 0 is a close call on 32 bit platforms.
>
> Still, it's not a new assumption in the code, so I'll grant you that.

What do you mean by "A close call"? All AVERROR_* are negative by
design, and they carry more information than a -1, so their increased
usage is certainly welcome.
The fact that it does not get propagated is a separate issue.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH] mpeg4videodec: only allow a positive length

[Vittorio Giovara]

On Wed, Apr 22, 2015 at 3:32 PM, Andreas Cadhalpun
 wrote:
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/mpeg4videodec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index 8449392..9bf33dd 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -189,14 +189,14 @@ static int 
> mpeg4_decode_sprite_trajectory(Mpeg4DecContext *ctx, GetBitContext *g
>  int x = 0, y = 0;
>
>  length = get_vlc2(gb, sprite_trajectory.table, SPRITE_TRAJ_VLC_BITS, 
> 3);
> -if (length)
> +if (length > 0)
>  x = get_xbits(gb, length);
>
>  if (!(ctx->divx_version == 500 && ctx->divx_build == 413))
>  check_marker(gb, "before sprite_trajectory");
>
>  length = get_vlc2(gb, sprite_trajectory.table, SPRITE_TRAJ_VLC_BITS, 
> 3);
> -if (length)
> +if (length > 0)
>  y = get_xbits(gb, length);
>
>  check_marker(gb, "after sprite_trajectory");
> --

Not very familiar with the code, but shouldn't you error out in this case?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH] dss_sp: use lowercase codec name without whitespace

[Vittorio Giovara]

On Wed, Apr 22, 2015 at 1:42 PM, Andreas Cadhalpun
 wrote:
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/dss_sp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/dss_sp.c b/libavcodec/dss_sp.c
> index 42ba1c4..909ad1f 100644
> --- a/libavcodec/dss_sp.c
> +++ b/libavcodec/dss_sp.c
> @@ -776,7 +776,7 @@ static int dss_sp_decode_frame(AVCodecContext *avctx, 
> void *data,
>  }
>
>  AVCodec ff_dss_sp_decoder = {
> -.name   = "DSS SP",
> +.name   = "dss_sp",
>  .long_name  = NULL_IF_CONFIG_SMALL("Digital Speech Standard - 
> Standard Play mode (DSS SP)"),
>  .type   = AVMEDIA_TYPE_AUDIO,
>  .id = AV_CODEC_ID_DSS_SP,

Makes sense, thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [RFC] Exporting the alpha mode from decoders

[Vittorio Giovara]

On Fri, Feb 6, 2015 at 11:32 AM, wm4  wrote:
> This is a proposal for an API extension.
>
> Currently, some pixel formats support alpha, but whether the alpha
> component contains something useful or just garbage is not part of the
> pixel format definition. This applies at least to packed RGB formats,
> where the 4th component is either alpha or garbage depending on the
> context.
>
> This means that if a decoder outputs such a packed RGB format, an API
> user has no idea whether it has alpha or not. E.g. take PNG and
> Camtasia (tscc.c). PNG with alpha outputs AV_PIX_FMT_RGBA, and Camtasia
> can output AV_PIX_FMT_RGB32 (which is ARGB or BGRA depending on
> endian). Camtasia supports no alpha, and the alpha component literally
> contains garbage (AFAIK and judging by the single sample I've seen). An
> application decoding both of these can't know whether the output frame
> has alpha or not, unless every codec is special-cased.
>
> One possible solution is duplicating all these pixel formats, so you'd
> have e.g. AV_PIX_FMT_RGBA and AV_PIX_FMT_RGBX. But I think we all agree
> that we don't want more pixel formats.
>
> The other solution, which I want to advocate here, is adding a field to
> AVFrame that indicates the alpha mode. Something like:
>
> typedef enum AVAlphaMode {
> // if an alpha component is present, its function is unknown, and
> // it may be garbage
> AV_ALPHA_UNKNOWN,
> // the alpha component contains premultiplied alpha
> // (not sure if any file format actually uses this)
> AV_ALPHA_PREMULTIPLIED,
> // the alpha component contains straight alpha
> // (e.g. PNG)
> AV_ALPHA_STRAIGHT,
> } AVAlphaMode;
>
> typedef struct AVFrame {
> ...
> AVAlphaMode alpha_mode;
> } AVFrame;
>
> Thoughts?

The problem looks interesting. I am not sure samples with
premultiplied alpha exist (or what swscale does in that case).
Another approach could be to expand avframe->flag, in order to signal
when alpha channel contains garbage, rather than introducing a new
field.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mov: Disable XMP metadata by default

[Vittorio Giovara]

On Tue, Dec 9, 2014 at 10:15 PM, wm4  wrote:
> On Tue, 9 Dec 2014 21:33:41 +0100
> Reimar Döffinger  wrote:
>
>> On Tue, Dec 09, 2014 at 12:59:24PM +, Vittorio Giovara wrote:
>> > On Tue, Dec 9, 2014 at 8:04 AM, wm4  wrote:
>> > > On Tue,  9 Dec 2014 02:57:01 +0100
>> > > Michael Niedermayer  wrote:
>> > >
>> > > It seems very wrong to export this info optionally just because
>> > > ffmpeg.c is too stupid to handle it correctly.
>> >
>> > Not exporting saves a little bit of memory too, since you have to
>> > allocate the space where to export this metadata to too.
>> > We're often talking about more than hundreds of megabytes.
>
> WTF, > 100 MBs of XML? In media files?

Yep, it's not uncommon that xmp contains the all the save states of
the project files (eg the list of undo action), imports, outputs
proxies and other stuff. It gets to ridiculous dimensions pretty
quickly.

>> From that aspect it might make more sense to have a generic
>> option to not export metadata larger than a certain size for example.
>> Or some other system.
>
> Until now, the method for exporting large metadata-like blobs was
> adding new streams. E.g. picture attachments and fonts.

That would be neat. Until then, can we have this patch committed?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat: add dump_metadata_lines

[Vittorio Giovara]

On Wed, Dec 10, 2014 at 1:23 PM, wm4  wrote:
> On Wed, 10 Dec 2014 03:38:03 +0100
> Lukasz Marek  wrote:
>
>> W dniu środa, 10 grudnia 2014 Vittorio Giovara 
>> napisał(a):
>>
>> > On Tue, Dec 9, 2014 at 10:17 PM, wm4 >
>> > wrote:
>> > > On Tue,  9 Dec 2014 14:10:22 +0100
>> > > Michael Niedermayer > wrote:
>> > >
>> > >> TODO: bump version, update APIChanges
>> > >>
>> > >> Signed-off-by: Michael Niedermayer >
>> > >> ---
>> > >>  libavformat/avformat.h  |8 
>> > >>  libavformat/dump.c  |   24 ++--
>> > >>  libavformat/options_table.h |1 +
>> > >>  3 files changed, 27 insertions(+), 6 deletions(-)
>> > >>
>> > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> > >> index 2e54ed1..cbe3608 100644
>> > >> --- a/libavformat/avformat.h
>> > >> +++ b/libavformat/avformat.h
>> > >> @@ -1616,6 +1616,14 @@ typedef struct AVFormatContext {
>> > >>   */
>> > >>  char *format_whitelist;
>> > >>
>> > >> +/**
>> > >> + * Maximum number of lines per metadata tag to dump with av_log.
>> > >> + * -1 means default
>> > >> + * - encoding: unused
>> > >> + * - decoding: set by user through AVOptions (NO direct access)
>> > >> + */
>> > >> +int dump_metadata_lines;
>> > >> +
>> > >>  /*
>> > >>   * All fields below this line are not part of the public API. They
>> > >>   * may not be used outside of libavformat and can be changed and
>> > >> diff --git a/libavformat/dump.c b/libavformat/dump.c
>> > >> index 56b37ff..38286b8 100644
>> > >> --- a/libavformat/dump.c
>> > >> +++ b/libavformat/dump.c
>> > >> @@ -126,7 +126,7 @@ static void print_fps(double d, const char *postfix)
>> > >>  av_log(NULL, AV_LOG_INFO, "%1.0fk %s", d / 1000, postfix);
>> > >>  }
>> > >>
>> > >> -static void dump_metadata(void *ctx, AVDictionary *m, const char
>> > *indent)
>> > >> +static void dump_metadata(void *ctx, AVDictionary *m, const char
>> > *indent, int dump_metadata_lines_arg)
>> > >>  {
>> > >>  if (m && !(av_dict_count(m) == 1 && av_dict_get(m, "language",
>> > NULL, 0))) {
>> > >>  AVDictionaryEntry *tag = NULL;
>> > >> @@ -135,16 +135,28 @@ static void dump_metadata(void *ctx, AVDictionary
>> > *m, const char *indent)
>> > >>  while ((tag = av_dict_get(m, "", tag, AV_DICT_IGNORE_SUFFIX)))
>> > >>  if (strcmp("language", tag->key)) {
>> > >>  const char *p = tag->value;
>> > >> +int lines = 0;
>> > >> +int dump_metadata_lines = dump_metadata_lines_arg;
>> > >> +if (dump_metadata_lines == -1) {
>> > >> +dump_metadata_lines = strcmp("comment", tag->key)
>> > ? 1 : 25;
>> > >> +}
>> > >>  av_log(ctx, AV_LOG_INFO,
>> > >> "%s  %-16s: ", indent, tag->key);
>> > >>  while (*p) {
>> > >>  char tmp[256];
>> > >>  size_t len = strcspn(p, "\x8\xa\xb\xc\xd");
>> > >> +if (lines >= dump_metadata_lines) {
>> > >> +av_log(ctx, AV_LOG_INFO, "[%"SIZE_SPECIFIER"
>> > bytes ommited, use \'-dump_metadata_lines \' to see more]", 
>> > strlen(p));
>> > >> +break;
>> > >> +}
>> > >>  av_strlcpy(tmp, p, FFMIN(sizeof(tmp), len+1));
>> > >>  av_log(ctx, AV_LOG_INFO, "%s", tmp);
>> > >>  p += len;
>> > >>  if (*p == 0xd) av_log(ctx, AV_LOG_INFO, " ");
>> > >> -if (*p == 0xa) av_log(ctx, AV_LOG_INFO, "\n%s
>> > %-16s

Re: [FFmpeg-devel] [PATCH] avformat: add dump_metadata_lines

[Vittorio Giovara]

On Tue, Dec 9, 2014 at 10:17 PM, wm4  wrote:
> On Tue,  9 Dec 2014 14:10:22 +0100
> Michael Niedermayer  wrote:
>
>> TODO: bump version, update APIChanges
>>
>> Signed-off-by: Michael Niedermayer 
>> ---
>>  libavformat/avformat.h  |8 
>>  libavformat/dump.c  |   24 ++--
>>  libavformat/options_table.h |1 +
>>  3 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 2e54ed1..cbe3608 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -1616,6 +1616,14 @@ typedef struct AVFormatContext {
>>   */
>>  char *format_whitelist;
>>
>> +/**
>> + * Maximum number of lines per metadata tag to dump with av_log.
>> + * -1 means default
>> + * - encoding: unused
>> + * - decoding: set by user through AVOptions (NO direct access)
>> + */
>> +int dump_metadata_lines;
>> +
>>  /*
>>   * All fields below this line are not part of the public API. They
>>   * may not be used outside of libavformat and can be changed and
>> diff --git a/libavformat/dump.c b/libavformat/dump.c
>> index 56b37ff..38286b8 100644
>> --- a/libavformat/dump.c
>> +++ b/libavformat/dump.c
>> @@ -126,7 +126,7 @@ static void print_fps(double d, const char *postfix)
>>  av_log(NULL, AV_LOG_INFO, "%1.0fk %s", d / 1000, postfix);
>>  }
>>
>> -static void dump_metadata(void *ctx, AVDictionary *m, const char *indent)
>> +static void dump_metadata(void *ctx, AVDictionary *m, const char *indent, 
>> int dump_metadata_lines_arg)
>>  {
>>  if (m && !(av_dict_count(m) == 1 && av_dict_get(m, "language", NULL, 
>> 0))) {
>>  AVDictionaryEntry *tag = NULL;
>> @@ -135,16 +135,28 @@ static void dump_metadata(void *ctx, AVDictionary *m, 
>> const char *indent)
>>  while ((tag = av_dict_get(m, "", tag, AV_DICT_IGNORE_SUFFIX)))
>>  if (strcmp("language", tag->key)) {
>>  const char *p = tag->value;
>> +int lines = 0;
>> +int dump_metadata_lines = dump_metadata_lines_arg;
>> +if (dump_metadata_lines == -1) {
>> +dump_metadata_lines = strcmp("comment", tag->key) ? 1 : 
>> 25;
>> +}
>>  av_log(ctx, AV_LOG_INFO,
>> "%s  %-16s: ", indent, tag->key);
>>  while (*p) {
>>  char tmp[256];
>>  size_t len = strcspn(p, "\x8\xa\xb\xc\xd");
>> +if (lines >= dump_metadata_lines) {
>> +av_log(ctx, AV_LOG_INFO, "[%"SIZE_SPECIFIER" bytes 
>> ommited, use \'-dump_metadata_lines \' to see more]", strlen(p));
>> +break;
>> +}
>>  av_strlcpy(tmp, p, FFMIN(sizeof(tmp), len+1));
>>  av_log(ctx, AV_LOG_INFO, "%s", tmp);
>>  p += len;
>>  if (*p == 0xd) av_log(ctx, AV_LOG_INFO, " ");
>> -if (*p == 0xa) av_log(ctx, AV_LOG_INFO, "\n%s  %-16s: 
>> ", indent, "");
>> +if (*p == 0xa) {
>> +av_log(ctx, AV_LOG_INFO, "\n%s  %-16s: ", indent, 
>> "");
>> +lines++;
>> +}
>>  if (*p) p++;
>>  }
>>  av_log(ctx, AV_LOG_INFO, "\n");
>> @@ -420,7 +432,7 @@ static void dump_stream_format(AVFormatContext *ic, int 
>> i,
>>  av_log(NULL, AV_LOG_INFO, " (clean effects)");
>>  av_log(NULL, AV_LOG_INFO, "\n");
>>
>> -dump_metadata(NULL, st->metadata, "");
>> +dump_metadata(NULL, st->metadata, "", ic->dump_metadata_lines);
>>
>>  dump_sidedata(NULL, st, "");
>>  }
>> @@ -438,7 +450,7 @@ void av_dump_format(AVFormatContext *ic, int index,
>> index,
>> is_output ? ic->oformat->name : ic->iformat->name,
>> is_output ? "to" : "from", url);
>> -dump_metadata(NULL, ic->metadata, "  ");
>> +dump_metadata(NULL, ic->metadata, "  ", ic->dump_metadata_lines);
>>
>>  if (!is_output) {
>>  av_log(NULL, AV_LOG_INFO, "  Duration: ");
>> @@ -480,7 +492,7 @@ void av_dump_format(AVFormatContext *ic, int index,
>>  av_log(NULL, AV_LOG_INFO,
>> "end %f\n", ch->end * av_q2d(ch->time_base));
>>
>> -dump_metadata(NULL, ch->metadata, "");
>> +dump_metadata(NULL, ch->metadata, "", ic->dump_metadata_lines);
>>  }
>>
>>  if (ic->nb_programs) {
>> @@ -490,7 +502,7 @@ void av_dump_format(AVFormatContext *ic, int index,
>>"name", NULL, 0);
>>  av_log(NULL, AV_LOG_INFO, "  Program %d %s\n", 
>> ic->programs[j]->id,
>> name ? name->value : "");
>> -dump_metadata(NULL, ic->programs[j]->metadata, "");
>> +

Re: [FFmpeg-devel] [PATCH] avformat/mov: Disable XMP metadata by default

[Vittorio Giovara]

On Tue, Dec 9, 2014 at 8:04 AM, wm4  wrote:
> On Tue,  9 Dec 2014 02:57:01 +0100
> Michael Niedermayer  wrote:
>
> It seems very wrong to export this info optionally just because
> ffmpeg.c is too stupid to handle it correctly.

Not exporting saves a little bit of memory too, since you have to
allocate the space where to export this metadata to too.
We're often talking about more than hundreds of megabytes.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tinterlace: Favor using standard timebases for the output

[Vittorio Giovara]

On Tue, Dec 2, 2014 at 5:31 PM, Michael Niedermayer  wrote:
> On Mon, Dec 01, 2014 at 11:01:05PM +0100, Michael Niedermayer wrote:
>> Reported-by: Vittorio Giovara 
>> Inspired by discussion with Kieran Kunhya 
>> Signed-off-by: Michael Niedermayer 
>> ---
>>  libavfilter/tinterlace.h|1 +
>>  libavfilter/vf_tinterlace.c |   25 +++--
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>
> applied
>
> please tell me if any issues remain

maybe you'd better revert the patch on vf_interlace too

> About the name, iam not attached to tinterlace if people prefer a
> name without "interlace" in it.

that would be indeed nice
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tinterlace: Favor using standard timebases for the output

[Vittorio Giovara]

On Tue, Dec 2, 2014 at 4:02 PM, Michael Niedermayer  wrote:
> If theres a problem in tinterlace as you keep hinting at, could you
> please explain what this problem is or provide a testcase that shows
> the problem?

The problem is that tinterlace does not really interlace, not a single
mode does.
Just because the name of the functionality kinda reminds of
interlacing it makes no sense to keep trying adding the interlace
functionality, especially since you have another interlace filter that
does the right thing.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tinterlace: Favor using standard timebases for the output

[Vittorio Giovara]

On Mon, Dec 1, 2014 at 9:55 PM, Michael Niedermayer  wrote:
>
> I dont think the patch will apply cleanly there, also
> If correct timestamps are the main reason for vf_interlace, then why
> was tinterlace not fixed instead of taking a subset of its features
> and creating a new filter out of that ?

Because there was not a single mode of tinterlace that actually
interlaced properly. You could have named it 'vf_funwithlines' and it
would have been more accurate, while vf_interlace did the right thing
since the start. Also I like smaller filters that do just one thing
but they do it right.

It'd be nice that fixes were propagated to both regardless of their
source anyway.

>> This effectively limits interlacing to two framerates. What about pure
>> 30i?
>
> added

Ok this effectively limits interlacing to three framerates. I am quite
sure there are other weird framerates that are not listed and are
valid interlaced formats unfortunately.

>> What about some future (or past) framerate we didn't think of?
>> Listing all possible framerate combinations is simply not
>> maintainable.
>
> i very much hope that the number of interlaced frame rates will be
> finite, small and not growing.
> In case that turns out not to materialize that way some other
> heuristic can be used instead of a table.

I very much hope people will stop using interlacing ^^

>> If you reeally want interlaced vfr why don't just add a filter
>> option like "keep_timebase" or something? Imho, it would be enough to
>> revert the broken patches.
>
> i can revert it for vf_interlace if you like?

As you prefer.

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