diff mbox series

[FFmpeg-devel,1/3] avdevice/xv: change codec to wrapped avframe

Message ID 20200405233824.29682-1-cus@passwd.hu
State New
Headers show
Series [FFmpeg-devel,1/3] avdevice/xv: change codec to wrapped avframe
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Marton Balint April 5, 2020, 11:38 p.m. UTC
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavdevice/xv.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Nicolas George April 6, 2020, 12:31 p.m. UTC | #1
Marton Balint (12020-04-06):
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavdevice/xv.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

Thanks for working on this.

Adding support for the more efficient API is a good idea.

On the other hand, if applications use this device or a similar one
specifically (I do in some of mine, I am certainly not alone), they
would hardcode rawvideo. Therefore, we cannot remove support for
rawvideo.

Regards,
Carl Eugen Hoyos April 6, 2020, 2:37 p.m. UTC | #2
Am Mo., 6. Apr. 2020 um 14:31 Uhr schrieb Nicolas George <george@nsup.org>:
>
> Marton Balint (12020-04-06):
> > Signed-off-by: Marton Balint <cus@passwd.hu>
> > ---
> >  libavdevice/xv.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
>
> Thanks for working on this.
>
> Adding support for the more efficient API is a good idea.
>
> On the other hand, if applications use this device or a similar one
> specifically (I do in some of mine, I am certainly not alone), they
> would hardcode rawvideo. Therefore, we cannot remove support for
> rawvideo.

We have done that in the past (for output formats that are not
testing-only as xv).

A minor bump would be nice though.

Carl Eugen
Nicolas George April 6, 2020, 2:39 p.m. UTC | #3
Carl Eugen Hoyos (12020-04-06):
> We have done that in the past (for output formats that are not
> testing-only as xv).

Xv is not testing only. OpenGL even less. This would be breaking actual
features. What formats are you thinking of? Even if we did, it is
entirely possible we were wrong then.

Regards,
Carl Eugen Hoyos April 6, 2020, 3:39 p.m. UTC | #4
Am Mo., 6. Apr. 2020 um 16:39 Uhr schrieb Nicolas George <george@nsup.org>:
>
> Carl Eugen Hoyos (12020-04-06):
> > We have done that in the past (for output formats that are not
> > testing-only as xv).
>
> Xv is not testing only. OpenGL even less. This would be
> breaking actual features.

Is it possible to support both codecs?

> What formats are you thinking of? Even if we did, it is
> entirely possible we were wrong then.

Commit 64ceeac2 changed the only supported format for the
null muxer without any version bump.
(While this is a very important muxer for me, I agree that my
argumentation above is not completely compelling.)

Carl Eugen
Nicolas George April 6, 2020, 5:58 p.m. UTC | #5
Carl Eugen Hoyos (12020-04-06):
> Is it possible to support both codecs?

Of course it it possible. And this is exactly the right thing to do
(modulo the question of which raw frame API we keep).

> Commit 64ceeac2 changed the only supported format for the
> null muxer without any version bump.
> (While this is a very important muxer for me, I agree that my
> argumentation above is not completely compelling.)

null is a different case: nobody will use null specifically, if they use
it, they use it as a placeholder for generic code capable of handling
any muxer. And since it is generic code, it will handle any codec.

But anyway, you are reading this commit wrongly. It does not remove
support for rawvideo in null, rawvideo is still supported in null, as
any other codec. This commit only changes the default codec.

Regards,
Marton Balint April 6, 2020, 7:15 p.m. UTC | #6
On Mon, 6 Apr 2020, Nicolas George wrote:

> Marton Balint (12020-04-06):
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavdevice/xv.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> Thanks for working on this.
>
> Adding support for the more efficient API is a good idea.
>
> On the other hand, if applications use this device or a similar one
> specifically (I do in some of mine, I am certainly not alone), they
> would hardcode rawvideo. Therefore, we cannot remove support for
> rawvideo.

avdevice/xv and avdevice/opengl are rarely used outside of ffmpeg CLI 
invocations, so I don't think it is worth supporting the inferior 
rawvideo. If you feel strongly about this then let's drop rawvideo support 
after the API bump.

Regards,
Marton
Nicolas George April 6, 2020, 7:18 p.m. UTC | #7
Marton Balint (12020-04-06):
> avdevice/xv and avdevice/opengl are rarely used outside of ffmpeg CLI
> invocations, so I don't think it is worth supporting the inferior rawvideo.

They are used.

> If you feel strongly about this then let's drop rawvideo support after the
> API bump.

No, let's keep it.

Regards,
Carl Eugen Hoyos April 6, 2020, 8:58 p.m. UTC | #8
Am Mo., 6. Apr. 2020 um 19:58 Uhr schrieb Nicolas George <george@nsup.org>:
>
> Carl Eugen Hoyos (12020-04-06):
> > Is it possible to support both codecs?
>
> Of course it it possible. And this is exactly the right thing to do
> (modulo the question of which raw frame API we keep).
>
> > Commit 64ceeac2 changed the only supported format for the
> > null muxer without any version bump.
> > (While this is a very important muxer for me, I agree that my
> > argumentation above is not completely compelling.)
>
> null is a different case: nobody will use null specifically, if they use
> it, they use it as a placeholder for generic code capable of handling
> any muxer. And since it is generic code, it will handle any codec.
>
> But anyway, you are reading this commit wrongly. It does not remove
> support for rawvideo in null, rawvideo is still supported in null, as
> any other codec. This commit only changes the default codec.

Thank you, I had never realized this in all the years I (thought I) had
to specify --enable-decoder=rawvideo,wrapped_avframe...

Carl Eugen
Nicolas George April 6, 2020, 9:13 p.m. UTC | #9
Carl Eugen Hoyos (12020-04-06):
> Thank you, I had never realized this in all the years I (thought I) had
> to specify --enable-decoder=rawvideo,wrapped_avframe...

I am not sure exactly what you are saying. You would need the
corresponding encoder, of course, whatever it is. And if it's not the
default, specifying it would be necessary.

Regards,
Carl Eugen Hoyos April 6, 2020, 9:39 p.m. UTC | #10
Am Mo., 6. Apr. 2020 um 23:13 Uhr schrieb Nicolas George <george@nsup.org>:
>
> Carl Eugen Hoyos (12020-04-06):
> > Thank you, I had never realized this in all the years I (thought I) had
> > to specify --enable-decoder=rawvideo,wrapped_avframe...
>
> I am not sure exactly what you are saying.

I tried to explain that for many years, when I tested a regression and
hit mentioned commit (which happens often), I had to recompile
because I did not realize I could just force the rawvideo encoder.

Carl Eugen
diff mbox series

Patch

diff --git a/libavdevice/xv.c b/libavdevice/xv.c
index c3ed2e48bd..2c5f1a4432 100644
--- a/libavdevice/xv.c
+++ b/libavdevice/xv.c
@@ -113,8 +113,8 @@  static int xv_write_header(AVFormatContext *s)
 
     if (   s->nb_streams > 1
         || par->codec_type != AVMEDIA_TYPE_VIDEO
-        || par->codec_id   != AV_CODEC_ID_RAWVIDEO) {
-        av_log(s, AV_LOG_ERROR, "Only supports one rawvideo stream\n");
+        || par->codec_id   != AV_CODEC_ID_WRAPPED_AVFRAME) {
+        av_log(s, AV_LOG_ERROR, "Only supports one wrapped avframe stream\n");
         return AVERROR(EINVAL);
     }
 
@@ -321,13 +321,8 @@  static int write_picture(AVFormatContext *s, uint8_t *input_data[4],
 
 static int xv_write_packet(AVFormatContext *s, AVPacket *pkt)
 {
-    AVCodecParameters *par = s->streams[0]->codecpar;
-    uint8_t *data[4];
-    int linesize[4];
-
-    av_image_fill_arrays(data, linesize, pkt->data, par->format,
-                         par->width, par->height, 1);
-    return write_picture(s, data, linesize);
+    AVFrame *frame = (AVFrame *)pkt->data;
+    return write_picture(s, frame->data, frame->linesize);
 }
 
 static int xv_write_frame(AVFormatContext *s, int stream_index, AVFrame **frame,
@@ -375,7 +370,7 @@  AVOutputFormat ff_xv_muxer = {
     .long_name      = NULL_IF_CONFIG_SMALL("XV (XVideo) output device"),
     .priv_data_size = sizeof(XVContext),
     .audio_codec    = AV_CODEC_ID_NONE,
-    .video_codec    = AV_CODEC_ID_RAWVIDEO,
+    .video_codec    = AV_CODEC_ID_WRAPPED_AVFRAME,
     .write_header   = xv_write_header,
     .write_packet   = xv_write_packet,
     .write_uncoded_frame = xv_write_frame,