GNOME Bugzilla – Bug 791674
Continued improvements to AOM plugin
Last modified: 2018-10-15 17:49:41 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!
(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 !
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
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!
Created attachment 367430 [details] [review] aom: Fix leak in av1dec
Created attachment 367431 [details] [review] aom: Put av1enc config debugging under gst_av1_enc_debug namespace
Created attachment 367432 [details] [review] aom: Cleanup related methods for av1enc/av1dec
Created attachment 367433 [details] [review] aom: Deadline was removed from AV1
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
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?
Created attachment 367444 [details] [review] aom: Fix leak in av1dec
Review of attachment 367432 [details] [review]: Will be split up into a series of smaller commits
Created attachment 367454 [details] [review] aom: Deadline was removed from AV1
Created attachment 367456 [details] [review] aom: Add direct casts for GstAV1Enc and GstAV1Dec
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
Created attachment 367787 [details] [review] Implement flush for av1dec
Created attachment 367788 [details] [review] aom: Consistent naming between av1dec and av1enc
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?
@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.
Should we file a bug/feature-request for that then perhaps so they can add some API for it?
Created attachment 367791 [details] [review] aom: Fix all definite leaks in av1enc
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
Created attachment 367849 [details] [review] aom: Fix all definite leaks in av1enc
@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!
It looks like you accidentially attached the same patch again, not the new version.
Created attachment 367873 [details] [review] aom: Fix all definite leaks in av1enc Oops sorry about that @slomo, here is the actual patch
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!
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 on attachment 367788 [details] [review] aom: Consistent naming between av1dec and av1enc This does not apply to latest git master
Comment on attachment 367873 [details] [review] aom: Fix all definite leaks in av1enc This one also does not apply
Created attachment 367895 [details] [review] aom: Consistent naming between av1dec and av1enc
Created attachment 367896 [details] [review] aom: Fix all definite leaks in av1enc
Created attachment 367897 [details] [review] aom: Implement cpu-used in av1enc
Created attachment 367898 [details] [review] aom: Drop pointless cast from av1enc
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
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 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
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..
Same as in VPX :) I think we should try to keep the names of the settings in the underlying libraries.
Created attachment 368237 [details] [review] aom: Implement cpu-used in av1enc If that looks good will submit the rest of the flags
Comment on attachment 368237 [details] [review] aom: Implement cpu-used in av1enc Looks good to me
Comment on attachment 368237 [details] [review] aom: Implement cpu-used in av1enc Attachment 368237 [details] pushed as a8ffc56 - aom: Implement cpu-used in av1enc
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)
(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
Created attachment 372844 [details] [review] aomenc: Add support for 10/12bit decoding
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?
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!
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
(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? :)
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.
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.
> 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.
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.
Created attachment 373535 [details] [review] Add rate control property [2] Thanks for the review ocrete, updated the patch and made it apply aom configuration.
Created attachment 373846 [details] [review] av1enc: Add configurations updated configuration patch
Created attachment 373847 [details] [review] av1enc: fix compliation with removed defines Fix build failure on recent aom encoder.
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.
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
*** Bug 797286 has been marked as a duplicate of this bug. ***