| Message ID | 20211219192134.1296-11-dcnieho@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | dshow enhancements | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Diederick Niehorster: > The list returned by get_device_list now contains info about what media > type(s), if any, can be provided by each device. > > Signed-off-by: Diederick Niehorster <dcnieho@gmail.com> > --- > libavdevice/dshow.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c > index f25537db5c..fa3a06c077 100644 > --- a/libavdevice/dshow.c > +++ b/libavdevice/dshow.c > @@ -377,6 +377,11 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, > if (!device->device_name || !device->device_description) > goto fail1; > > + device->nb_media_types = nb_media_types; > + device->media_types = media_types; > + nb_media_types = 0; > + media_types = NULL; > + If this array is intended to be given to the caller, my "put in on the stack" suggestion from #8 is no longer feasible. > // store to device_list output > if (av_reallocp_array(&(*device_list)->devices, > (*device_list)->nb_devices + 1, > @@ -412,6 +417,8 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, > av_freep(&device->device_name); > if (device->device_name) > av_freep(&device->device_description); > + if (device->media_types) > + av_freep(&device->media_types); You are duplicating freeing code here: You have code to free media_types both before it was put into device and after; you can avoid the latter by only attaching media_types to device when nothing else can fail. Btw: All these checks before av_freep() here are unnecessary. > av_free(device); > } > if (olestr && co_malloc) >
On Mon, Dec 20, 2021 at 2:04 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Diederick Niehorster: > > // store to device_list output > > if (av_reallocp_array(&(*device_list)->devices, > > (*device_list)->nb_devices + 1, > > @@ -412,6 +417,8 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, > > av_freep(&device->device_name); > > if (device->device_name) > > av_freep(&device->device_description); > > + if (device->media_types) > > + av_freep(&device->media_types); > > You are duplicating freeing code here: You have code to free media_types > both before it was put into device and after; you can avoid the latter > by only attaching media_types to device when nothing else can fail. I could indeed avoid adding av_freep(&device->media_types) when only attaching media_types to the device once it made it into the device list array (so doing (*device_list)->devices[(*device_list)->nb_devices]->media_types = media_types). But that makes the code harder to read (asymmetric, everything else is attached to the device before it goes into the list) and the missing free while the other device members are freed looks like a code smell. I think this is easier to read and less error prone for future patches when done as currently. Thanks! Dee
Hi Andreas, On Mon, Dec 20, 2021 at 9:01 AM Diederick C. Niehorster <dcnieho@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 2:04 AM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: > > > > Diederick Niehorster: > > > // store to device_list output > > > if (av_reallocp_array(&(*device_list)->devices, > > > (*device_list)->nb_devices + 1, > > > @@ -412,6 +417,8 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, > > > av_freep(&device->device_name); > > > if (device->device_name) > > > av_freep(&device->device_description); > > > + if (device->media_types) > > > + av_freep(&device->media_types); > > > > You are duplicating freeing code here: You have code to free media_types > > both before it was put into device and after; you can avoid the latter > > by only attaching media_types to device when nothing else can fail. > > I could indeed avoid adding av_freep(&device->media_types) when only > attaching media_types to the device once it made it into the device > list array (so doing > (*device_list)->devices[(*device_list)->nb_devices]->media_types = > media_types). But that makes the code harder to read (asymmetric, > everything else is attached to the device before it goes into the > list) and the missing free while the other device members are freed > looks like a code smell. I think this is easier to read and less error > prone for future patches when done as currently. Nevermind, with some reordering, i see i can do as you suggest. Will do. Cheers, Dee
diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c index f25537db5c..fa3a06c077 100644 --- a/libavdevice/dshow.c +++ b/libavdevice/dshow.c @@ -377,6 +377,11 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, if (!device->device_name || !device->device_description) goto fail1; + device->nb_media_types = nb_media_types; + device->media_types = media_types; + nb_media_types = 0; + media_types = NULL; + // store to device_list output if (av_reallocp_array(&(*device_list)->devices, (*device_list)->nb_devices + 1, @@ -412,6 +417,8 @@ dshow_cycle_devices(AVFormatContext *avctx, ICreateDevEnum *devenum, av_freep(&device->device_name); if (device->device_name) av_freep(&device->device_description); + if (device->media_types) + av_freep(&device->media_types); av_free(device); } if (olestr && co_malloc)
The list returned by get_device_list now contains info about what media type(s), if any, can be provided by each device. Signed-off-by: Diederick Niehorster <dcnieho@gmail.com> --- libavdevice/dshow.c | 7 +++++++ 1 file changed, 7 insertions(+)