After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 791674 - Continued improvements to AOM plugin
Continued improvements to AOM plugin
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 797286 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-12-16 06:18 UTC by Sean-Der
Modified: 2018-10-15 17:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
aom: Fix leak in av1dec (661 bytes, patch)
2018-01-25 17:32 UTC, Sean-Der
none Details | Review
aom: Put av1enc config debugging under gst_av1_enc_debug namespace (2.24 KB, patch)
2018-01-25 17:33 UTC, Sean-Der
committed Details | Review
aom: Cleanup related methods for av1enc/av1dec (15.88 KB, patch)
2018-01-25 17:33 UTC, Sean-Der
none Details | Review
aom: Deadline was removed from AV1 (2.31 KB, patch)
2018-01-25 17:33 UTC, Sean-Der
none Details | Review
aom: Fix leak in av1dec (693 bytes, patch)
2018-01-25 22:50 UTC, Sean-Der
committed Details | Review
aom: Deadline was removed from AV1 (1.35 KB, patch)
2018-01-26 05:30 UTC, Sean-Der
committed Details | Review
aom: Add direct casts for GstAV1Enc and GstAV1Dec (4.35 KB, patch)
2018-01-26 06:03 UTC, Sean-Der
committed Details | Review
Implement flush for av1dec (1.84 KB, patch)
2018-02-02 06:24 UTC, Sean-Der
committed Details | Review
aom: Consistent naming between av1dec and av1enc (4.00 KB, patch)
2018-02-02 06:24 UTC, Sean-Der
none Details | Review
aom: Fix all definite leaks in av1enc (5.03 KB, patch)
2018-02-02 08:22 UTC, Sean-Der
none Details | Review
aom: Fix all definite leaks in av1enc (5.03 KB, patch)
2018-02-03 06:55 UTC, Sean-Der
none Details | Review
aom: Fix all definite leaks in av1enc (5.80 KB, patch)
2018-02-04 08:56 UTC, Sean-Der
none Details | Review
aom: Implement cpu-used in av1enc (4.44 KB, patch)
2018-02-05 00:22 UTC, Sean-Der
reviewed Details | Review
aom: Consistent naming between av1dec and av1enc (4.00 KB, patch)
2018-02-05 09:01 UTC, Sean-Der
committed Details | Review
aom: Fix all definite leaks in av1enc (5.80 KB, patch)
2018-02-05 09:02 UTC, Sean-Der
committed Details | Review
aom: Implement cpu-used in av1enc (3.79 KB, patch)
2018-02-05 09:03 UTC, Sean-Der
none Details | Review
aom: Drop pointless cast from av1enc (1.10 KB, patch)
2018-02-05 09:05 UTC, Sean-Der
committed Details | Review
aom: Implement cpu-used in av1enc (5.34 KB, patch)
2018-02-11 22:26 UTC, Sean-Der
committed Details | Review
aomenc: Add support for 10/12bit decoding (5.54 KB, patch)
2018-06-27 09:44 UTC, Sean-Der
committed Details | Review
aomenc: Handle 8 bit_depth images with AOM_IMG_FMT_HIGHBITDEPTH enabled (1.98 KB, patch)
2018-06-27 09:48 UTC, Sean-Der
committed Details | Review
av1enc: Add rate control properties (22.67 KB, patch)
2018-07-30 15:09 UTC, Wonchul Lee
none Details | Review
Add rate control property [2] (27.12 KB, patch)
2018-09-01 03:35 UTC, Wonchul Lee
none Details | Review
av1enc: Add configurations (28.80 KB, patch)
2018-10-04 14:36 UTC, Wonchul Lee
committed Details | Review
av1enc: fix compliation with removed defines (1010 bytes, patch)
2018-10-04 14:40 UTC, Wonchul Lee
committed Details | Review
av1enc: Add to configure image formats (5.79 KB, patch)
2018-10-07 07:48 UTC, Wonchul Lee
committed Details | Review

Description Sean-Der 2017-12-16 06:18:31 UTC
The AOM plugin was merged into bad, but there is still plenty of work left to be done. These are the following things that need to finished.

https://bugzilla.gnome.org/show_bug.cgi?id=784160
----

* Properly handle EOS:
** av1enc: in the finish vmethod you want to drain out the remaining frames
** av1dec: Same thing, but in both the finish and drain vmethod

* gobject properties for configuring the encoder :)

* Maybe have some "sane" default settings on the encoder

* I tried, but failed, tu get the encoder to use multiple-threads for encoding. Might be worth checking what happens there.

* As asked before, I didn't see any "out-of-band" data that needs to be passed around. Is that normal for av1 ?

* Tests

----

I took care of world domination, I guess I have to take care of the rest...

 Edward I saw you cleaned up after me a little bit, are you planning on doing anything more (I do not want to step on your toes) I am planning on moving top/bottom through this list. So will post a patch for EOS handling here soon.

Sorry I don't understand what you mean by 'out-of-band' data is this for the encoding or decoding? I know we need to track one variable for requesting keyframes (the interval isn't an encoder option) but beyond that I am not aware. Maybe more will be needed as I fill everything out.

I am also going to reach out to the AOM IRC and announce we support AV1 and maybe get their feedback (make sure the properties all look right) eventually

thanks!
Comment 1 Edward Hervey 2017-12-18 14:49:20 UTC
(In reply to Sean-Der from comment #0)
> I took care of world domination, I guess I have to take care of the rest...

  :)

> 
>  Edward I saw you cleaned up after me a little bit, are you planning on
> doing anything more (I do not want to step on your toes) I am planning on
> moving top/bottom through this list. So will post a patch for EOS handling
> here soon.

  go ahead, if ever I come up with an itch to scratch I'll post it here. I might write a parser element "for fun"

> 
> Sorry I don't understand what you mean by 'out-of-band' data is this for the
> encoding or decoding? I know we need to track one variable for requesting
> keyframes (the interval isn't an encoder option) but beyond that I am not
> aware. Maybe more will be needed as I fill everything out.

  It would be the stream-long configuration which doesn't change. Ex for h264 it's SPS/PPS. I'm not that familiar with AV1, but since vp8/vp9 don't have such things as codec_data ... let's just ignore that issue for now.

> 
> I am also going to reach out to the AOM IRC and announce we support AV1 and
> maybe get their feedback (make sure the properties all look right) eventually

  Great !
Comment 2 Sean-Der 2018-01-01 21:54:29 UTC
Hey

I took care of properly handling EOS (and cleaned up all the leaks, issues I found along the way) once these are merged I am gonna add all the properties. Hopefully I did everything right! 


I don't want to spam everyone (but I can post the patches here as well if you want) but feel free to squash this as well. I just commit when I get a chunk of work done, so I don't blow things away when debugging by accident.

https://github.com/Sean-Der/gst-plugins-bad/commit/e5562a6faaefcd7d0805854ddf9b2faca62033d6.patch

https://github.com/Sean-Der/gst-plugins-bad/commit/1ab9c65907ea62d763e33e8c4e0e3ac8d799e8b9.patch

https://github.com/Sean-Der/gst-plugins-bad/commit/47f054712fbad1922edcf13f3042fc4b94148376.patch

https://github.com/Sean-Der/gst-plugins-bad/commit/79c5e0efdfba8d5af71a396f065944e16d418a65.patch

https://github.com/Sean-Der/gst-plugins-bad/commit/99883a2e4065a692b29e99b191a210e1459bbfff.patch
Comment 3 Sean-Der 2018-01-14 22:26:49 UTC
Hey Edward!

I squashed the commits down like we talked about over IRC, and did better commit messages. There are no other changes, if you want me to look in anything before they can be merged just tell me! Otherwise after they are merged I will do all the properties, and reach out and get sane defaults.

https://github.com/Sean-Der/gst-plugins-bad/commit/e5562a6faaefcd7d0805854ddf9b2faca62033d6.patch

https://github.com/Sean-Der/gst-plugins-bad/commit/c40c91a43b60e9421414574f3744daa92af8c58b.patch

https://github.com/Sean-Der/gst-plugins-bad/commit/fd7c605baa65b786755bf7a507b615e71fe11d5f.patch


thanks!
Comment 4 Sean-Der 2018-01-25 17:32:49 UTC
Created attachment 367430 [details] [review]
aom: Fix leak in av1dec
Comment 5 Sean-Der 2018-01-25 17:33:23 UTC
Created attachment 367431 [details] [review]
aom: Put av1enc config debugging under gst_av1_enc_debug namespace
Comment 6 Sean-Der 2018-01-25 17:33:38 UTC
Created attachment 367432 [details] [review]
aom: Cleanup related methods for av1enc/av1dec
Comment 7 Sean-Der 2018-01-25 17:33:53 UTC
Created attachment 367433 [details] [review]
aom: Deadline was removed from AV1
Comment 8 Sebastian Dröge (slomo) 2018-01-25 17:55:50 UTC
Review of attachment 367430 [details] [review]:

::: ext/aom/gstav1dec.c
@@ +180,3 @@
+  if (av1dec->decoder_inited) {
+    aom_codec_destroy (&av1dec->decoder);
+  }

You probably also want to set decoder_inited = FALSE here
Comment 9 Sebastian Dröge (slomo) 2018-01-25 18:04:12 UTC
Review of attachment 367432 [details] [review]:

::: ext/aom/gstav1dec.c
@@ +193,3 @@
 gst_av1_dec_set_format (GstVideoDecoder * dec, GstVideoCodecState * state)
 {
+  GstAV1Dec *av1dec = GST_AV1_DEC (dec);

These changes seem like cleanup that should probably happen in a separate commit. Usually we define a GST_AV1_DEC_CAST() for the direct cast without runtime type checks

@@ +383,3 @@
+      GST_WARNING_OBJECT (av1dec, "%s", warn);
+    }
+    aom_img_free (img);

draining/finishing should both forward all pending frames downstream. flushing should drop all pending frames and reset the codec.

::: ext/aom/gstav1enc.c
@@ +149,3 @@
+
+  av1enc->encoder_inited = FALSE;
+  av1enc->deadline = AOM_DL_GOOD_QUALITY;

You just add this here to remove it again in the next commit :)

@@ +262,3 @@
+  av1enc->aom_cfg.g_h = GST_VIDEO_INFO_HEIGHT (&av1enc->input_state->info);
+  av1enc->aom_cfg.g_timebase.num = 1;
+  av1enc->aom_cfg.g_timebase.den = 90000;

Why? Also these changes seem all unrelated to what you wrote in the commit message.

Doing it like in vpxenc would make sense here IMHO

@@ -349,3 @@
     flags |= AOM_EFLAG_FORCE_KF;
   }
-  av1enc->keyframe_dist++;

You're removing the regular keyframes here. Is there automatic keyframe-creation in the codec now? Can a max keyframe distance be configured?

@@ +379,3 @@
+
+    if (duration > 0) {
+      av1enc->last_pts += frame->duration;

What do we need the last_pts for? By adding up frame durations you add up rounding errors

@@ +434,3 @@
+  }
+
+  return gst_av1_enc_process (av1enc);

here's only ever a single frame left in the codec? Shouldn't this be a loop?
Comment 10 Sean-Der 2018-01-25 22:50:59 UTC
Created attachment 367444 [details] [review]
aom: Fix leak in av1dec
Comment 11 Sean-Der 2018-01-26 05:28:57 UTC
Review of attachment 367432 [details] [review]:

Will be split up into a series of smaller commits
Comment 12 Sean-Der 2018-01-26 05:30:50 UTC
Created attachment 367454 [details] [review]
aom: Deadline was removed from AV1
Comment 13 Sean-Der 2018-01-26 06:03:14 UTC
Created attachment 367456 [details] [review]
aom: Add direct casts for GstAV1Enc and GstAV1Dec
Comment 14 Sebastian Dröge (slomo) 2018-01-26 09:13:39 UTC
Attachment 367431 [details] pushed as 0900cbd - aom: Put av1enc config debugging under gst_av1_enc_debug namespace
Attachment 367444 [details] pushed as c89f610 - aom: Fix leak in av1dec
Attachment 367454 [details] pushed as 8b27637 - aom: Deadline was removed from AV1
Attachment 367456 [details] pushed as 2b03bc0 - aom: Add direct casts for GstAV1Enc and GstAV1Dec
Comment 15 Sean-Der 2018-02-02 06:24:28 UTC
Created attachment 367787 [details] [review]
Implement flush for av1dec
Comment 16 Sean-Der 2018-02-02 06:24:59 UTC
Created attachment 367788 [details] [review]
aom: Consistent naming between av1dec and av1enc
Comment 17 Sebastian Dröge (slomo) 2018-02-02 06:29:39 UTC
Comment on attachment 367787 [details] [review]
Implement flush for av1dec

There's no nicer way in the API to reset the codec other than completely recreating it?

Also the codec will never have any pending frames queued up that have to be drained first?
Comment 18 Sean-Der 2018-02-02 07:38:09 UTC
@slomo

I don't believe there is anything else we can do (best to destroy and re-create), I looked through the API and I don't see anything.

Also nothing with pending frames will exist when decoding, only encoding.
Comment 19 Tim-Philipp Müller 2018-02-02 08:08:31 UTC
Should we file a bug/feature-request for that then perhaps so they can add some API for it?
Comment 20 Sean-Der 2018-02-02 08:22:20 UTC
Created attachment 367791 [details] [review]
aom: Fix all definite leaks in av1enc
Comment 21 Sebastian Dröge (slomo) 2018-02-02 12:15:57 UTC
Review of attachment 367791 [details] [review]:

::: ext/aom/gstav1enc.c
@@ +139,3 @@
   GST_PAD_SET_ACCEPT_TEMPLATE (GST_VIDEO_ENCODER_SINK_PAD (av1enc));
+
+  av1enc->keyframe_dist = 30;

I don't like this manual, fixed keyframe dist but that's for later I guess :)

@@ +149,3 @@
+
+  if (av1enc->input_state) {
+    gst_video_codec_state_unref (av1enc->input_state);

It should always be unreffed in stop(). It can only exist >= start()

@@ +240,3 @@
+  output_state =
+      gst_video_encoder_set_output_state (encoder,
+      gst_pad_get_pad_template_caps (GST_VIDEO_ENCODER_SRC_PAD (encoder)),

The caps need to be unreffed

@@ +244,3 @@
+  gst_video_codec_state_unref (output_state);
+
+  av1enc->input_state = gst_video_codec_state_ref (state);

If there already is an input_state, you first should unref it
Comment 22 Sean-Der 2018-02-03 06:55:02 UTC
Created attachment 367849 [details] [review]
aom: Fix all definite leaks in av1enc
Comment 23 Sean-Der 2018-02-03 06:56:37 UTC
@slomo
Update per your review! Thanks for checking it out, I made sure that output_state isn't lost and input_state is unref'ed before setting it again.

The hardcoded keyframe will be fixed next :) now that we have the base stuff down I am going to start adding properties once this lands (basic ones like cpu-used and keyframe will make a huge difference)

thanks!
Comment 24 Sebastian Dröge (slomo) 2018-02-04 07:11:19 UTC
It looks like you accidentially attached the same patch again, not the new version.
Comment 25 Sean-Der 2018-02-04 08:56:06 UTC
Created attachment 367873 [details] [review]
aom: Fix all definite leaks in av1enc

Oops sorry about that @slomo, here is the actual patch
Comment 26 Sean-Der 2018-02-05 00:22:38 UTC
Created attachment 367891 [details] [review]
aom: Implement cpu-used in av1enc

If this patch looks good, can I just add all the other properties in a similar fashion (and in one commit)

After that it should just be flushing the encoder and tests, and then I feel confident that it is 'MVP worthy'

thanks!
Comment 27 Sebastian Dröge (slomo) 2018-02-05 07:44:53 UTC
Review of attachment 367891 [details] [review]:

::: ext/aom/gstav1enc.c
@@ +131,3 @@
+  g_object_class_install_property (gobject_class, PROP_CPU_USED,
+      g_param_spec_int ("cpu-used", "CPU Used",
+          "CPU Used. A Value greater than 0 will increase encoder speed at the expense of quality.",

Not at the expense of CPU usage?

@@ +181,1 @@
+  GST_WARNING_OBJECT (av1enc, "Latency unimplemented");

Why these unrelated changes? Better put them into a separate commit :)

@@ +429,3 @@
+    GST_WARNING_OBJECT (av1enc,
+        "Properties can only be changed when NULL or READY");
+    GST_OBJECT_UNLOCK (av1enc);

I don't think so? cpu-used looks like it can be changed at any time, and things like bitrate certainly will be possible.
Comment 28 Sebastian Dröge (slomo) 2018-02-05 08:09:56 UTC
Comment on attachment 367788 [details] [review]
aom: Consistent naming between av1dec and av1enc

This does not apply to latest git master
Comment 29 Sebastian Dröge (slomo) 2018-02-05 08:10:47 UTC
Comment on attachment 367873 [details] [review]
aom: Fix all definite leaks in av1enc

This one also does not apply
Comment 30 Sean-Der 2018-02-05 09:01:44 UTC
Created attachment 367895 [details] [review]
aom: Consistent naming between av1dec and av1enc
Comment 31 Sean-Der 2018-02-05 09:02:12 UTC
Created attachment 367896 [details] [review]
aom: Fix all definite leaks in av1enc
Comment 32 Sean-Der 2018-02-05 09:03:29 UTC
Created attachment 367897 [details] [review]
aom: Implement cpu-used in av1enc
Comment 33 Sean-Der 2018-02-05 09:05:07 UTC
Created attachment 367898 [details] [review]
aom: Drop pointless cast from av1enc
Comment 34 Sean-Der 2018-02-05 09:15:12 UTC
Blargh sorry about that @slomo 

Split up the latency issue per review, and rebased against master they all apply.

re: cpu-used wording I copied directly from AV1 [0]

I put the NULL/ready check in because to do that while playing, I would need a mutex around the encoder. Seems like a good chunk of work and I was gonna make it its own patch, it will be the very next thing I submit. But didn't want to confuse anyone using it right now (hey I am setting cpu-used why is it a no-op)

Or I can make it part of the cpu-used patch!

https://github.com/mozilla/aom/blob/c8b38b0bfd36306f1886375d98512d792ffe9517/aom/aomcx.h#L137-L138
Comment 35 Sebastian Dröge (slomo) 2018-02-05 13:35:09 UTC
Attachment 367895 [details] pushed as 327586b - aom: Consistent naming between av1dec and av1enc
Attachment 367896 [details] pushed as 86abe6b - aom: Fix all definite leaks in av1enc
Attachment 367898 [details] pushed as fc4fe1c - aom: Drop pointless cast from av1enc
Comment 36 Sebastian Dröge (slomo) 2018-02-05 13:37:07 UTC
Comment on attachment 367897 [details] [review]
aom: Implement cpu-used in av1enc

I would prefer the usage of a mutex and allowing to change the settings in PLAYING if possible. You should be able to re-use the object lock here, but take a look at vpx to see how it's done there.

Apart from that this patch looks fine
Comment 37 Tim-Philipp Müller 2018-02-05 15:29:45 UTC
I'm a bit confused by the "cpu-used" property. The name is a bit odd IMHO, it sounds like a read-only property that returns cpu usage..
Comment 38 Sebastian Dröge (slomo) 2018-02-05 15:31:54 UTC
Same as in VPX :) I think we should try to keep the names of the settings in the underlying libraries.
Comment 39 Sean-Der 2018-02-11 22:26:00 UTC
Created attachment 368237 [details] [review]
aom: Implement cpu-used in av1enc

If that looks good will submit the rest of the flags
Comment 40 Sebastian Dröge (slomo) 2018-02-13 18:33:41 UTC
Comment on attachment 368237 [details] [review]
aom: Implement cpu-used in av1enc

Looks good to me
Comment 41 Sebastian Dröge (slomo) 2018-02-14 08:20:34 UTC
Comment on attachment 368237 [details] [review]
aom: Implement cpu-used in av1enc

Attachment 368237 [details] pushed as a8ffc56 - aom: Implement cpu-used in av1enc
Comment 42 Sean-Der 2018-06-26 08:20:52 UTC
And we are back! I recently changed jobs, but am back working on this again sorry for the radio silence.

These are the things I will be landing patches for.

* mp4 support (https://aomediacodec.github.io/av1-isobmff/)
* Finish out the other encoder flags
* Support all pixel formats (and error out on high bit depth like vpx)
Comment 43 Sebastian Dröge (slomo) 2018-06-26 13:19:18 UTC
(In reply to Sean-Der from comment #42)

> * Support all pixel formats (and error out on high bit depth like vpx)

See https://bugzilla.gnome.org/show_bug.cgi?id=773208
Comment 44 Sean-Der 2018-06-27 09:44:14 UTC
Created attachment 372844 [details] [review]
aomenc: Add support for 10/12bit decoding
Comment 45 Sean-Der 2018-06-27 09:48:07 UTC
Created attachment 372845 [details] [review]
aomenc: Handle 8 bit_depth images with AOM_IMG_FMT_HIGHBITDEPTH enabled

This one is really important, if you have to prioritize I would do this one first. Without this patch decoding of most video files are broken.

This flag was turned off by default for a while, but now is on (AOM_IMG_FMT_HIGHBITDEPTH) it just means in the 8bit case we have to hit skip the unused 8.

Putting the check inside gst_av1_dec_image_to_buffer might not be ok. If you prefer I can create a dedicated function for handling this case.

gst_av1_dec_image_to_buffer AND gst_av1_dec_8bit_image_to_buffer something like that?
Comment 46 Sean-Der 2018-06-29 05:34:55 UTC
Hey @slomo!

Anything I can do to help get this in (clean up the patches, better way to submit them?)

I don't want to block the GSOC work at all. I assume they will be using this stuff. After these two get merged I can take my time with mp4.

thanks!
Comment 47 Sebastian Dröge (slomo) 2018-06-29 05:48:25 UTC
Attachment 372844 [details] pushed as 1d96d9e - aomenc: Add support for 10/12bit decoding
Attachment 372845 [details] pushed as 10a37e0 - aomenc: Handle 8 bit_depth images with AOM_IMG_FMT_HIGHBITDEPTH enabled
Comment 48 Sebastian Dröge (slomo) 2018-06-29 05:49:50 UTC
(In reply to Sean-Der from comment #46)
> Hey @slomo!
> 
> Anything I can do to help get this in (clean up the patches, better way to
> submit them?)

Nothing really, it just takes time to get patches reviewed and merged and there are lots of them :)

> I don't want to block the GSOC work at all. I assume they will be using this
> stuff. After these two get merged I can take my time with mp4.

Why would you block the GSoC work? :)
Comment 49 Sean-Der 2018-06-29 07:14:05 UTC
Awesome! Just want to make sure I am doing a good job.

Without 'Handle 8 bit_depth' in some cases videos will improperly decode, I was just worried that f15h would be trying to use av1dec and think it was an issue with their code.

But since that is in, all is good :D thanks again for making it so easy to contribute.
Comment 50 Wonchul Lee 2018-07-30 15:09:18 UTC
Created attachment 373219 [details] [review]
av1enc: Add rate control properties

Hello, I was tracking av1 codec related changes and it seems not to be updated for a moment.
So if you do not mind, I'd like to continue making patches that I can do for av1.

I just added rate control of av1enc properties on this patch. Since av1enc currently uses the default configuration, these properties will not be applied right away, and it seems to be added all relevant properties together. If you guys think this patch looks ok, I'm going to collaborate with this work to complete.
Comment 51 Tim-Philipp Müller 2018-07-30 15:30:57 UTC
> After these two get merged I can take my time with mp4.

Just to be sure, Olivier has already done some work on the av1 mapping in qtmux/qtdemux in git master, so be sure to check that out before starting from scratch.
Comment 52 Olivier Crête 2018-07-30 19:47:32 UTC
Review of attachment 373219 [details] [review]:

::: ext/aom/gstav1enc.c
@@ +99,3 @@
+    {AOM_CBR, "cbr", "cbr"},
+    {AOM_CQ, "cq", "cq"},
+    {AOM_Q, "q", "q"},

The long form should be something human understandable (cq isn't for example).

@@ +245,3 @@
+      g_param_spec_uint ("drop-frame", "Drop frame",
+          "Temporal resampling configuration, Set to zero (0) to disable this \
+          featrue.", 0, G_MAXUINT, DROP_FRAME_DEFAULT, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

s/featrue/feature/ .. And it probably makes sense to add a little bit more explanation of what this means.

@@ +795,3 @@
+    g_mutex_unlock (&av1enc->encoder_lock);
+    if (status != AOM_CODEC_OK) {
+      GST_ELEMENT_ERROR (av1enc, LIBRARY, INIT,

This should probably be a warning instead of an error, it's not fatal per-se. And it's not INIT.. and I'm not sure it should be an message, mayube a simple GST_WARNING_OBJECT() is enough.
Comment 53 Wonchul Lee 2018-09-01 03:35:52 UTC
Created attachment 373535 [details] [review]
Add rate control property [2]

Thanks for the review ocrete, updated the patch and made it apply aom configuration.
Comment 54 Wonchul Lee 2018-10-04 14:36:54 UTC
Created attachment 373846 [details] [review]
av1enc: Add configurations

updated configuration patch
Comment 55 Wonchul Lee 2018-10-04 14:40:25 UTC
Created attachment 373847 [details] [review]
av1enc: fix compliation with removed defines

Fix build failure on recent aom encoder.
Comment 56 Wonchul Lee 2018-10-07 07:48:00 UTC
Created attachment 373862 [details] [review]
av1enc: Add to configure image formats

It expands to support image format to YV12/I422/I444, bit-depth and adjust profile according to the format and bit-depth.
In terms of bit-depth, if the bit-depth of the input stream can be analyzed in a format other than y4m, there is room for improvement to utilize it.
Comment 57 Olivier Crête 2018-10-10 19:47:03 UTC
Merged the patches, let's close this bug. If you have more generic improvements just re-open it. If it's something specific we maybe are better opening a new one.

Attachment 373846 [details] pushed as 7d73307 - av1enc: Add configurations
Attachment 373847 [details] pushed as cc9d65a - av1enc: fix compliation with removed defines
Attachment 373862 [details] pushed as 51d5db3 - av1enc: Add to configure image formats
Comment 58 Olivier Crête 2018-10-15 17:49:41 UTC
*** Bug 797286 has been marked as a duplicate of this bug. ***